Attachment 246353 Details for Bug 141451 – [PATCH] Proposed Fix

[patch] [PATCH] Proposed Fix

stylesheet-mutation.patch (text/plain), 9.41 KB, created by Joseph Pecoraro on 2015-02-10 17:32:43 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Joseph Pecoraro

Created: 2015-02-10 17:32:43 PST

Size: 9.41 KB

patch

obsolete

>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 01a5abc..4397e90 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,15 @@
>+2015-02-10  Joseph Pecoraro  <pecoraro@apple.com>
>+
>+        Web Inspector: Crash inspecting styles of element with mutated stylesheet
>+        https://bugs.webkit.org/show_bug.cgi?id=141451
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * inspector/css/stylesheet-with-mutations-expected.txt: Added.
>+        * inspector/css/stylesheet-with-mutations.html: Added.
>+        Previously this test would crash, or in debug builds show one
>+        of the two selectors in the rule.
>+
> 2015-02-09  Brent Fulgham  <bfulgham@apple.com>
> 
>         [Win] Activate media tests. (Unreviewed)
>diff --git a/LayoutTests/inspector/css/stylesheet-with-mutations-expected.txt b/LayoutTests/inspector/css/stylesheet-with-mutations-expected.txt
>new file mode 100644
>index 0000000..0d7c79f
>--- /dev/null
>+++ b/LayoutTests/inspector/css/stylesheet-with-mutations-expected.txt
>@@ -0,0 +1,5 @@
>+Testing that we get expected values for selectors after CSSOM stylesheet modifications.
>+
>+.foo
>+.bar
>+
>diff --git a/LayoutTests/inspector/css/stylesheet-with-mutations.html b/LayoutTests/inspector/css/stylesheet-with-mutations.html
>new file mode 100644
>index 0000000..e584ed1
>--- /dev/null
>+++ b/LayoutTests/inspector/css/stylesheet-with-mutations.html
>@@ -0,0 +1,58 @@
>+<!DOCTYPE html>
>+<html>
>+<head>
>+<style>
>+.foo, .bar { color: red; }
>+</style>
>+<script type="text/javascript" src="../../http/tests/inspector/inspector-test.js"></script>
>+<script>
>+// Insert a rule at the beginning of the inline stylesheet above that would shift all rule ordinals down.
>+document.styleSheets[0].insertRule("body { padding: 10px; }", 0);
>+
>+function test() {
>+    var nodeStyles;
>+
>+    function validateSelectors()
>+    {
>+        for (var i = 0; i < nodeStyles.matchedRules.length; ++i) {
>+            var rule = nodeStyles.matchedRules[i];
>+            if (rule.type !== WebInspector.CSSRule.Type.Author)
>+                continue;
>+
>+            for (var selector of rule.selectors)
>+                InspectorTest.log(selector.text);
>+        }
>+
>+        InspectorTest.completeTest();
>+    }
>+
>+    function onStylesRefreshed()
>+    {
>+        nodeStyles.removeEventListener(WebInspector.DOMNodeStyles.Event.Refreshed, onStylesRefreshed, this);
>+        validateSelectors();
>+    }
>+
>+    WebInspector.domTreeManager.requestDocument(function(documentNode) {
>+        WebInspector.domTreeManager.querySelector(documentNode.id, ".bar", function(contentNodeId) {
>+            if (contentNodeId) {
>+                var domNode = WebInspector.domTreeManager.nodeForId(contentNodeId);
>+                nodeStyles = WebInspector.cssStyleManager.stylesForNode(domNode);
>+
>+                if (nodeStyles.needsRefresh)
>+                    nodeStyles.addEventListener(WebInspector.DOMNodeStyles.Event.Refreshed, onStylesRefreshed, this);
>+                else
>+                    validateSelectors();
>+            } else {
>+                InspectorTest.log("DOM node not found.");
>+                InspectorTest.completeTest();
>+            }
>+        });
>+    });
>+}
>+</script>
>+</head>
>+<body onload="runTest()">
>+    <p>Testing that we get expected values for selectors after a stylesheet has mutations.</p>
>+    <div class="bar"></div>
>+</body>
>+</html>
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index a708a4b..1f04c6e 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,40 @@
>+2015-02-10  Joseph Pecoraro  <pecoraro@apple.com>
>+
>+        Web Inspector: Crash inspecting styles of element with mutated stylesheet
>+        https://bugs.webkit.org/show_bug.cgi?id=141451
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Test: inspector/css/stylesheet-with-mutations.html
>+
>+        * css/CSSStyleSheet.h:
>+        * css/CSSStyleSheet.cpp:
>+        (WebCore::CSSStyleSheet::CSSStyleSheet):
>+        (WebCore::CSSStyleSheet::didMutateRules):
>+        Keep a flag on the CSSStyleSheet to know when it has had a rule
>+        mutation from the contents.
>+
>+        * inspector/InspectorStyleSheet.h:
>+        * inspector/InspectorStyleSheet.cpp:
>+        (WebCore::InspectorStyleSheet::reparseStyleSheet):
>+        When completely replacing the stylesheet's contents, we will have
>+        destroyed any previous rules, so clear the has mutations flag.
>+
>+        (WebCore::selectorsFromSource):
>+        Previously there was a mismatch between the rules the parsed
>+        SourceData had, and the actual CSSSelectors we were iterating.
>+        We use the SourceData so we can get the exact user authored
>+        text instead of generated longhands from actualy selector objects.
>+        Add an ASSERT and bail to catch and more gracefully handle
>+        any possible mismatches in the future.
>+
>+        (WebCore::InspectorStyleSheet::styleSheetMutated):
>+        (WebCore::InspectorStyleSheet::ensureParsedDataReady):
>+        When a CSSStyleSheet has been mutated beyond the inspector's
>+        knowledge right now, fall back to readonly generated selectors.
>+        We should better handle this in the future:
>+        <https://webkit.org/b/141450> Web Inspector: Better support for CSSOM StyleSheet mutations (insertRule/deleteRule)
>+
> 2015-02-10  Carlos Garcia Campos  <cgarcia@igalia.com>
> 
>         [GTK] GMutexLocker build issue
>diff --git a/Source/WebCore/css/CSSStyleSheet.cpp b/Source/WebCore/css/CSSStyleSheet.cpp
>index 7f70207..6b1b4c8 100644
>--- a/Source/WebCore/css/CSSStyleSheet.cpp
>+++ b/Source/WebCore/css/CSSStyleSheet.cpp
>@@ -95,6 +95,7 @@ CSSStyleSheet::CSSStyleSheet(Ref<StyleSheetContents>&& contents, CSSImportRule*
>     : m_contents(WTF::move(contents))
>     , m_isInlineStylesheet(false)
>     , m_isDisabled(false)
>+    , m_mutatedRules(false)
>     , m_ownerNode(0)
>     , m_ownerRule(ownerRule)
> {
>@@ -105,6 +106,7 @@ CSSStyleSheet::CSSStyleSheet(Ref<StyleSheetContents>&& contents, Node* ownerNode
>     : m_contents(WTF::move(contents))
>     , m_isInlineStylesheet(isInlineStylesheet)
>     , m_isDisabled(false)
>+    , m_mutatedRules(false)
>     , m_ownerNode(ownerNode)
>     , m_ownerRule(0)
> {
>@@ -177,6 +179,8 @@ void CSSStyleSheet::didMutateRules(RuleMutationType mutationType, WhetherContent
>     }
> 
>     owner->styleResolverChanged(DeferRecalcStyle);
>+
>+    m_mutatedRules = true;
> }
> 
> void CSSStyleSheet::didMutate()
>diff --git a/Source/WebCore/css/CSSStyleSheet.h b/Source/WebCore/css/CSSStyleSheet.h
>index 1890539..d312eec 100644
>--- a/Source/WebCore/css/CSSStyleSheet.h
>+++ b/Source/WebCore/css/CSSStyleSheet.h
>@@ -88,6 +88,9 @@ public:
>     void setMediaQueries(PassRefPtr<MediaQuerySet>);
>     void setTitle(const String& title) { m_title = title; }
> 
>+    bool hadRulesMutation() const { return m_mutatedRules; }
>+    void clearHadRulesMutation() { m_mutatedRules = false; }
>+
>     enum RuleMutationType { OtherMutation, RuleInsertion };
>     enum WhetherContentsWereClonedForMutation { ContentsWereNotClonedForMutation = 0, ContentsWereClonedForMutation };
> 
>@@ -129,6 +132,7 @@ private:
>     Ref<StyleSheetContents> m_contents;
>     bool m_isInlineStylesheet;
>     bool m_isDisabled;
>+    bool m_mutatedRules;
>     String m_title;
>     RefPtr<MediaQuerySet> m_mediaQueries;
> 
>diff --git a/Source/WebCore/inspector/InspectorStyleSheet.cpp b/Source/WebCore/inspector/InspectorStyleSheet.cpp
>index 6f82376..62ed67a 100644
>--- a/Source/WebCore/inspector/InspectorStyleSheet.cpp
>+++ b/Source/WebCore/inspector/InspectorStyleSheet.cpp
>@@ -609,6 +609,9 @@ void InspectorStyleSheet::reparseStyleSheet(const String& text)
>         m_pageStyleSheet->clearChildRuleCSSOMWrappers();
>         fireStyleSheetChanged();
>     }
>+
>+    // We just wiped the entire contents of the stylesheet. Clear the mutation flag.
>+    m_pageStyleSheet->clearHadRulesMutation();
> }
> 
> bool InspectorStyleSheet::setText(const String& text, ExceptionCode& ec)
>@@ -860,6 +863,12 @@ static Ref<Inspector::Protocol::Array<Inspector::Protocol::CSS::CSSSelector>> se
>     const SelectorRangeList& ranges = sourceData->selectorRanges;
>     const CSSSelector* selector = selectorList.first();
>     for (size_t i = 0, size = ranges.size(); i < size; ++i) {
>+        // If we don't have a selector, that means the SourceData for this CSSStyleSheet
>+        // no longer matches up with the actual rules in the CSSStyleSheet.
>+        ASSERT(selector);
>+        if (!selector)
>+            break;
>+
>         const SourceRange& range = ranges.at(i);
>         String selectorText = sheetText.substring(range.start, range.length());
> 
>@@ -1061,9 +1070,14 @@ bool InspectorStyleSheet::checkPageStyleSheet(ExceptionCode& ec) const
>     return true;
> }
> 
>+bool InspectorStyleSheet::styleSheetMutated() const
>+{
>+    return m_pageStyleSheet && m_pageStyleSheet->hadRulesMutation();
>+}
>+
> bool InspectorStyleSheet::ensureParsedDataReady()
> {
>-    return ensureText() && ensureSourceData();
>+    return !styleSheetMutated() && ensureText() && ensureSourceData();
> }
> 
> bool InspectorStyleSheet::ensureText() const
>diff --git a/Source/WebCore/inspector/InspectorStyleSheet.h b/Source/WebCore/inspector/InspectorStyleSheet.h
>index e95dd60..9a82187 100644
>--- a/Source/WebCore/inspector/InspectorStyleSheet.h
>+++ b/Source/WebCore/inspector/InspectorStyleSheet.h
>@@ -215,6 +215,7 @@ private:
> 
>     static void collectFlatRules(RefPtr<CSSRuleList>&&, CSSStyleRuleVector* result);
>     bool checkPageStyleSheet(ExceptionCode&) const;
>+    bool styleSheetMutated() const;
>     bool ensureText() const;
>     bool ensureSourceData();
>     void ensureFlatRules() const;

Flags:

buildbot: commit-queue-

Actions: View | Formatted Diff | Diff

Attachments on bug 141451: 246353 | 246361 | 246362 | 246364