Changeset 212614 in webkit

Timestamp:

Feb 19, 2017 4:12:07 AM (7 years ago)

Author:

Antti Koivisto

Message:

Execute pending scripts asynchronously after stylesheet loads complete
​https://bugs.webkit.org/show_bug.cgi?id=168367
rdar://problem/30561379

Reviewed by Andreas Kling.

The current synchronous execution is fragile and creates various problems.

• css/StyleSheetContents.cpp:

(WebCore::StyleSheetContents::checkLoaded):

• dom/ContainerNode.cpp:

(WebCore::ContainerNode::takeAllChildrenFrom):
(WebCore::ContainerNode::notifyChildInserted):
(WebCore::ContainerNode::removeChild):
(WebCore::ContainerNode::parserRemoveChild):
(WebCore::ContainerNode::removeChildren):

Remove various places where we would trigger delayed synchronous execution.

• dom/Document.cpp:

(WebCore::Document::Document):
(WebCore::Document::recalcStyle):

Trigger scroll to anchor at the end of style resolution instead of when style sheet load completes.

(WebCore::Document::didRemoveAllPendingStylesheet):

Call asynchronous script execution function.

• dom/Document.h:

(WebCore::Document::setNeedsNotifyRemoveAllPendingStylesheet): Deleted.
(WebCore::Document::notifyRemovePendingSheetIfNeeded): Deleted.

• dom/ScriptableDocumentParser.cpp:

(WebCore::ScriptableDocumentParser::ScriptableDocumentParser):
(WebCore::ScriptableDocumentParser::executeScriptsWaitingForStylesheetsSoon):
(WebCore::ScriptableDocumentParser::scriptsWaitingForStylesheetsExecutionTimerFired):

Add a timer for executing pending scripts.

(WebCore::ScriptableDocumentParser::detach):

• dom/ScriptableDocumentParser.h:

(WebCore::ScriptableDocumentParser::executeScriptsWaitingForStylesheets):

• html/HTMLLinkElement.cpp:

(WebCore::HTMLLinkElement::removedFrom):
(WebCore::HTMLLinkElement::removePendingSheet):

• html/HTMLLinkElement.h:
• html/parser/HTMLDocumentParser.cpp:

(WebCore::HTMLDocumentParser::detach):

• loader/DocumentLoader.cpp:

(WebCore::DocumentLoader::isLoadingInAPISense):

Stay in loading state if we have a pending script. This matches existing behavior.

• style/StyleScope.cpp:

(WebCore::Style::Scope::removePendingSheet):

• style/StyleScope.h:

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• css/StyleSheetContents.cpp (modified) (1 diff)
• dom/ContainerNode.cpp (modified) (5 diffs)
• dom/Document.cpp (modified) (6 diffs)
• dom/Document.h (modified) (5 diffs)
• dom/ScriptableDocumentParser.cpp (modified) (3 diffs)
• dom/ScriptableDocumentParser.h (modified) (3 diffs)
• html/HTMLLinkElement.cpp (modified) (3 diffs)
• html/HTMLLinkElement.h (modified) (1 diff)
• html/parser/HTMLDocumentParser.cpp (modified) (2 diffs)
• html/parser/HTMLDocumentParser.h (modified) (1 diff)
• loader/DocumentLoader.cpp (modified) (2 diffs)
• loader/FrameLoader.cpp (modified) (1 diff)
• style/StyleScope.cpp (modified) (2 diffs)
• style/StyleScope.h (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2017-02-19  Antti Koivisto  <antti@apple.com>
+
+        Execute pending scripts asynchronously after stylesheet loads complete
+        https://bugs.webkit.org/show_bug.cgi?id=168367
+        rdar://problem/30561379
+
+        Reviewed by Andreas Kling.
+
+        The current synchronous execution is fragile and creates various problems.
+
+        * css/StyleSheetContents.cpp:
+        (WebCore::StyleSheetContents::checkLoaded):
+        * dom/ContainerNode.cpp:
+        (WebCore::ContainerNode::takeAllChildrenFrom):
+        (WebCore::ContainerNode::notifyChildInserted):
+        (WebCore::ContainerNode::removeChild):
+        (WebCore::ContainerNode::parserRemoveChild):
+        (WebCore::ContainerNode::removeChildren):
+
+            Remove various places where we would trigger delayed synchronous execution.
+
+        * dom/Document.cpp:
+        (WebCore::Document::Document):
+        (WebCore::Document::recalcStyle):
+
+            Trigger scroll to anchor at the end of style resolution instead of when style sheet load completes.
+
+        (WebCore::Document::didRemoveAllPendingStylesheet):
+
+            Call asynchronous script execution function.
+
+        * dom/Document.h:
+        (WebCore::Document::setNeedsNotifyRemoveAllPendingStylesheet): Deleted.
+        (WebCore::Document::notifyRemovePendingSheetIfNeeded): Deleted.
+        * dom/ScriptableDocumentParser.cpp:
+        (WebCore::ScriptableDocumentParser::ScriptableDocumentParser):
+        (WebCore::ScriptableDocumentParser::executeScriptsWaitingForStylesheetsSoon):
+        (WebCore::ScriptableDocumentParser::scriptsWaitingForStylesheetsExecutionTimerFired):
+
+            Add a timer for executing pending scripts.
+
+        (WebCore::ScriptableDocumentParser::detach):
+        * dom/ScriptableDocumentParser.h:
+        (WebCore::ScriptableDocumentParser::executeScriptsWaitingForStylesheets):
+        * html/HTMLLinkElement.cpp:
+        (WebCore::HTMLLinkElement::removedFrom):
+        (WebCore::HTMLLinkElement::removePendingSheet):
+        * html/HTMLLinkElement.h:
+        * html/parser/HTMLDocumentParser.cpp:
+        (WebCore::HTMLDocumentParser::detach):
+        * loader/DocumentLoader.cpp:
+        (WebCore::DocumentLoader::isLoadingInAPISense):
+
+            Stay in loading state if we have a pending script. This matches existing behavior.
+
+        * style/StyleScope.cpp:
+        (WebCore::Style::Scope::removePendingSheet):
+        * style/StyleScope.h:
+
 2017-02-18  Chris Dumez  <cdumez@apple.com>
 

trunk/Source/WebCore/css/StyleSheetContents.cpp

@@ -365 +365 @@
         return;
 
-    // Avoid |this| being deleted by scripts that run via
-    // ScriptableDocumentParser::executeScriptsWaitingForStylesheets().
-    // See <rdar://problem/6622300>.
     Ref<StyleSheetContents> protectedThis(*this);
     StyleSheetContents* parentSheet = parentStyleSheet();

trunk/Source/WebCore/dom/ContainerNode.cpp

@@ -146 +146 @@
     }
 
-    document().notifyRemovePendingSheetIfNeeded();
-
     // FIXME: assert that we don't dispatch events here since this container node is still disconnected.
     for (auto& child : children) {
@@ -365 +363 @@
     childrenChanged(change);
 
-    // Some call sites may have deleted child nodes. Calling it earlier results in bugs like webkit.org/b/168069.
-    document().notifyRemovePendingSheetIfNeeded();
-
     for (auto& target : postInsertionNotificationTargets)
         target->finishedInsertingSubtree();
@@ -555 +550 @@
     }
 
-    document().notifyRemovePendingSheetIfNeeded();
     rebuildSVGExtensionsElementsIfNecessary();
     dispatchSubtreeModifiedEvent();
@@ -616 +610 @@
 
     notifyChildRemoved(oldChild, prev, next, ChildChangeSourceParser);
-    document().notifyRemovePendingSheetIfNeeded();
 }
 
@@ -710 +703 @@
     }
 
-    document().notifyRemovePendingSheetIfNeeded();
     rebuildSVGExtensionsElementsIfNecessary();
     dispatchSubtreeModifiedEvent();

trunk/Source/WebCore/dom/Document.cpp

@@ -444 +444 @@
     , m_settings(frame ? Ref<Settings>(frame->settings()) : Settings::create(nullptr))
     , m_hasNodesWithPlaceholderStyle(false)
-    , m_needsNotifyRemoveAllPendingStylesheet(false)
     , m_ignorePendingStylesheets(false)
     , m_pendingSheetLayout(NoLayoutWithPendingSheets)
@@ -1847 +1846 @@
     if (m_hoveredElement && !m_hoveredElement->renderer())
         frameView.frame().mainFrame().eventHandler().dispatchFakeMouseMoveEventSoon();
+
+    if (m_gotoAnchorNeededAfterStylesheetsLoad && !styleScope().hasPendingSheets())
+        frameView.scrollToFragment(m_url);
 }
 
@@ -2624 +2626 @@
     }
 
-    m_frame->loader().checkCompleted();
+    checkCompleted();
 }
 
@@ -3074 +3076 @@
 void Document::didRemoveAllPendingStylesheet()
 {
-    m_needsNotifyRemoveAllPendingStylesheet = false;
-
     if (m_pendingSheetLayout == DidLayoutWithPendingSheets) {
+        // Painting is disabled when doing layouts with pending sheets to avoid FOUC.
+        // We need to force paint when coming out from this state.
+        // FIXME: This is not very elegant.
         m_pendingSheetLayout = IgnoreLayoutWithPendingSheets;
         if (renderView())
@@ -3082 +3085 @@
     }
 
-    if (ScriptableDocumentParser* parser = scriptableDocumentParser())
-        parser->executeScriptsWaitingForStylesheets();
-
-    if (m_gotoAnchorNeededAfterStylesheetsLoad && view())
-        view()->scrollToFragment(m_url);
+    if (auto* parser = scriptableDocumentParser())
+        parser->executeScriptsWaitingForStylesheetsSoon();
 }
 
@@ -6039 +6039 @@
 void Document::loadEventDelayTimerFired()
 {
-    if (frame())
-        frame()->loader().checkCompleted();
+    checkCompleted();
+}
+
+void Document::checkCompleted()
+{
+    if (auto* frame = this->frame())
+        frame->loader().checkCompleted();
 }
 

trunk/Source/WebCore/dom/Document.h

@@ -487 +487 @@
     CSSFontSelector& fontSelector() { return m_fontSelector; }
 
-    void notifyRemovePendingSheetIfNeeded();
-
     WEBCORE_EXPORT bool haveStylesheetsLoaded() const;
 
@@ -1117 +1115 @@
     void decrementLoadEventDelayCount();
     bool isDelayingLoadEvent() const { return m_loadEventDelayCount; }
+    void checkCompleted();
 
 #if ENABLE(IOS_TOUCH_EVENTS)
@@ -1207 +1206 @@
 
     void didRemoveAllPendingStylesheet();
-    void setNeedsNotifyRemoveAllPendingStylesheet() { m_needsNotifyRemoveAllPendingStylesheet = true; }
     void didClearStyleResolver();
 
@@ -1392 +1390 @@
     std::unique_ptr<StyleResolver> m_userAgentShadowTreeStyleResolver;
     bool m_hasNodesWithPlaceholderStyle;
-    bool m_needsNotifyRemoveAllPendingStylesheet;
     // But sometimes you need to ignore pending stylesheet count to
     // force an immediate layout when requested by JS.
@@ -1758 +1755 @@
 Element* eventTargetElementForDocument(Document*);
 
-inline void Document::notifyRemovePendingSheetIfNeeded()
-{
-    if (m_needsNotifyRemoveAllPendingStylesheet)
-        didRemoveAllPendingStylesheet();
-}
-
 inline TextEncoding Document::textEncoding() const
 {

trunk/Source/WebCore/dom/ScriptableDocumentParser.cpp

@@ -29 +29 @@
 #include "Document.h"
 #include "Settings.h"
+#include "StyleScope.h"
 
 namespace WebCore {
@@ -36 +37 @@
     , m_wasCreatedByScript(false)
     , m_parserContentPolicy(parserContentPolicy)
+    , m_scriptsWaitingForStylesheetsExecutionTimer(*this, &ScriptableDocumentParser::scriptsWaitingForStylesheetsExecutionTimerFired)
 {
     if (!pluginContentIsAllowed(m_parserContentPolicy))
@@ -44 +46 @@
 }
 
+void ScriptableDocumentParser::executeScriptsWaitingForStylesheetsSoon()
+{
+    ASSERT(!document()->styleScope().hasPendingSheets());
+
+    if (m_scriptsWaitingForStylesheetsExecutionTimer.isActive())
+        return;
+    if (!hasScriptsWaitingForStylesheets())
+        return;
+
+    m_scriptsWaitingForStylesheetsExecutionTimer.startOneShot(0);
+}
+
+void ScriptableDocumentParser::scriptsWaitingForStylesheetsExecutionTimerFired()
+{
+    ASSERT(!isDetached());
+
+    Ref<ScriptableDocumentParser> protectedThis(*this);
+
+    if (!document()->styleScope().hasPendingSheets())
+        executeScriptsWaitingForStylesheets();
+
+    if (!isDetached())
+        document()->checkCompleted();
+}
+
+void ScriptableDocumentParser::detach()
+{
+    m_scriptsWaitingForStylesheetsExecutionTimer.stop();
+
+    DecodedDataDocumentParser::detach();
+}
+
 };

trunk/Source/WebCore/dom/ScriptableDocumentParser.h

@@ -28 +28 @@
 #include "DecodedDataDocumentParser.h"
 #include "FragmentScriptingPermission.h"
+#include "Timer.h"
 #include <wtf/text/TextPosition.h>
 
@@ -38 +39 @@
     virtual bool isExecutingScript() const { return false; }
 
-    // FIXME: Only the HTMLDocumentParser ever blocks script execution on
-    // stylesheet load, which is likely a bug in the XMLDocumentParser.
-    virtual void executeScriptsWaitingForStylesheets() { }
-
     virtual bool isWaitingForScripts() const = 0;
 
     virtual TextPosition textPosition() const = 0;
+
+    virtual bool hasScriptsWaitingForStylesheets() const { return false; }
+
+    void executeScriptsWaitingForStylesheetsSoon();
 
     // Returns true if the parser didn't yield or pause or synchronously execute a script,
@@ -58 +59 @@
     explicit ScriptableDocumentParser(Document&, ParserContentPolicy = AllowScriptingContent);
 
+    virtual void executeScriptsWaitingForStylesheets() { }
+
+    void detach() override;
+
 private:
     ScriptableDocumentParser* asScriptableDocumentParser() final { return this; }
+
+    void scriptsWaitingForStylesheetsExecutionTimerFired();
 
     // http://www.whatwg.org/specs/web-apps/current-work/#script-created-parser
     bool m_wasCreatedByScript;
     ParserContentPolicy m_parserContentPolicy;
+    Timer m_scriptsWaitingForStylesheetsExecutionTimer;
 };
 

trunk/Source/WebCore/html/HTMLLinkElement.cpp

@@ -328 +328 @@
 
     if (styleSheetIsLoading())
-        removePendingSheet(RemovePendingSheetNotifyLater);
+        removePendingSheet();
     
     if (m_styleScope) {
@@ -555 +555 @@
 }
 
-void HTMLLinkElement::removePendingSheet(RemovePendingSheetNotificationType notification)
+void HTMLLinkElement::removePendingSheet()
 {
     PendingSheetType type = m_pendingSheetType;
@@ -570 +570 @@
     }
 
-    m_styleScope->removePendingSheet(
-        notification == RemovePendingSheetNotifyImmediately
-        ? Style::Scope::RemovePendingSheetNotifyImmediately
-        : Style::Scope::RemovePendingSheetNotifyLater);
+    m_styleScope->removePendingSheet();
 }
 

trunk/Source/WebCore/html/HTMLLinkElement.h

@@ -112 +112 @@
     void addPendingSheet(PendingSheetType);
 
-    enum RemovePendingSheetNotificationType {
-        RemovePendingSheetNotifyImmediately,
-        RemovePendingSheetNotifyLater
-    };
-
-    void removePendingSheet(RemovePendingSheetNotificationType = RemovePendingSheetNotifyImmediately);
+    void removePendingSheet();
 
     LinkLoader m_linkLoader;

trunk/Source/WebCore/html/parser/HTMLDocumentParser.cpp

@@ -88 +88 @@
 void HTMLDocumentParser::detach()
 {
-    DocumentParser::detach();
+    ScriptableDocumentParser::detach();
 
     if (m_scriptRunner)
@@ -539 +539 @@
 }
 
+bool HTMLDocumentParser::hasScriptsWaitingForStylesheets() const
+{
+    return m_scriptRunner && m_scriptRunner->hasScriptsWaitingForStylesheets();
+}
+
 void HTMLDocumentParser::executeScriptsWaitingForStylesheets()
 {

trunk/Source/WebCore/html/parser/HTMLDocumentParser.h

@@ -84 +84 @@
     bool isWaitingForScripts() const override;
     bool isExecutingScript() const final;
+    bool hasScriptsWaitingForStylesheets() const final;
     void executeScriptsWaitingForStylesheets() final;
     void suspendScheduledTasks() final;

trunk/Source/WebCore/loader/DocumentLoader.cpp

@@ -69 +69 @@
 #include "ResourceLoadObserver.h"
 #include "SchemeRegistry.h"
+#include "ScriptableDocumentParser.h"
 #include "SecurityPolicy.h"
 #include "Settings.h"
@@ -1058 +1059 @@
         if (document.hasActiveParser())
             return true;
+        auto* scriptableParser = document.scriptableDocumentParser();
+        if (scriptableParser && scriptableParser->hasScriptsWaitingForStylesheets())
+            return true;
     }
     return frameLoader()->subframeIsLoading();

trunk/Source/WebCore/loader/FrameLoader.cpp

@@ -788 +788 @@
         return;
 
+    auto* scriptableParser = m_frame.document()->scriptableDocumentParser();
+    if (scriptableParser && scriptableParser->hasScriptsWaitingForStylesheets())
+        return;
+
     // Any frame that hasn't completed yet?
     if (!allChildrenAreComplete())

trunk/Source/WebCore/style/StyleScope.cpp

@@ -171 +171 @@
 
 // This method is called whenever a top-level stylesheet has finished loading.
-void Scope::removePendingSheet(RemovePendingSheetNotificationType notification)
+void Scope::removePendingSheet()
 {
     // Make sure we knew this sheet was pending, and that our count isn't out of sync.
@@ -179 +179 @@
     if (m_pendingStyleSheetCount)
         return;
-
-    if (notification == RemovePendingSheetNotifyLater) {
-        m_document.setNeedsNotifyRemoveAllPendingStylesheet();
-        return;
-    }
 
     didChangeActiveStyleSheetCandidates();

trunk/Source/WebCore/style/StyleScope.h

@@ -83 +83 @@
 
     void addPendingSheet() { m_pendingStyleSheetCount++; }
-    enum RemovePendingSheetNotificationType {
-        RemovePendingSheetNotifyImmediately,
-        RemovePendingSheetNotifyLater
-    };
-    void removePendingSheet(RemovePendingSheetNotificationType = RemovePendingSheetNotifyImmediately);
+    void removePendingSheet();
 
     bool hasPendingSheets() const { return m_pendingStyleSheetCount > 0; }