Bug 130653 – Remove code in HTMLObjectElement attribute parsing that forces style resolution and layout

• Component: Page Loading
• Status: RESOLVED FIXED

[Darin Adler]

Remove code in HTMLObjectElement attribute parsing that forces style resolution and layout

[Darin Adler]

Created attachment 227613 [details]
Patch

[WebKit Commit Bot]

Attachment 227613 [details] did not pass style-queue:


ERROR: Source/WebCore/html/HTMLPlugInImageElement.cpp:280:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/html/HTMLFormControlElement.cpp:293:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/html/HTMLFrameOwnerElement.cpp:128:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/style/StyleResolveTree.h:52:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/style/StyleResolveTree.cpp:908:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/style/StyleResolveTree.cpp:910:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/style/StyleResolveTree.cpp:914:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/style/StyleResolveTree.cpp:928:  Missing space before {  [whitespace/braces] [5]
Total errors found: 8 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Style bot doesn’t like the way I am formatting lambdas. Do we need to fix it?

[Darin Adler]

Comment on attachment 227613 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=227613&action=review

> Source/WebCore/html/HTMLObjectElement.cpp:375
> +    // This is here mainly to keep acid2 non-flaky. A style recalc is required to make fallback resources. Without forcing

Oops, this should say "resources load." not "resources." and also not "resources to load."

[Darin Adler]

Created attachment 227616 [details]
Patch

[Antti Koivisto]

Comment on attachment 227613 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=227613&action=review

> Source/WebCore/html/HTMLFormControlElement.cpp:294
> +            if (auto renderer = element->renderer())

auto*

> Source/WebCore/html/HTMLPlugInImageElement.cpp:333
>  void HTMLPlugInImageElement::didMoveToNewDocument(Document* oldDocument)
>  {
>      if (m_needsDocumentActivationCallbacks) {
> -        if (oldDocument)
> -            oldDocument->unregisterForPageCacheSuspensionCallbacks(this);
> +        oldDocument->unregisterForPageCacheSuspensionCallbacks(this);
>          document().registerForPageCacheSuspensionCallbacks(this);
>      }
>  
>      if (m_imageLoader)
>          m_imageLoader->elementDidMoveToNewDocument();
> +    if (m_needsImageReload) {
> +        oldDocument->decrementLoadEventDelayCount();

As discussed decrement is missing the case where the element is removed from the document. This could leave load event permanently blocked.

> Source/WebCore/html/HTMLPlugInImageElement.cpp:805
> +    document().incrementLoadEventDelayCount();

Wish these increment/decrements were encapsulated. Code that expects calls like these to pair in all cases tends to be fragile.

[WebKit Commit Bot]

Attachment 227616 [details] did not pass style-queue:


ERROR: Source/WebCore/html/HTMLPlugInImageElement.cpp:280:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/html/HTMLFormControlElement.cpp:293:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/html/HTMLFrameOwnerElement.cpp:128:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/style/StyleResolveTree.h:52:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/style/StyleResolveTree.cpp:908:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/style/StyleResolveTree.cpp:910:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/style/StyleResolveTree.cpp:914:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/style/StyleResolveTree.cpp:928:  Missing space before {  [whitespace/braces] [5]
Total errors found: 8 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexey Proskuryakov]

This was landed in <http://trac.webkit.org/changeset/166144>, and seems to have caused bug 130942.

Is this something that can be rolled out?

[Darin Adler]

(In reply to comment #8)
> Is this something that can be rolled out?

I don’t think a lot of other tests depend on it, but Andreas Kling told me it provided a 10% speedup to an important performance test, so I’d prefer not to roll it out without further research.

[Stephanie Lewis]

Rolled out in http://trac.webkit.org/changeset/166680 due to issues with the PLT <rdar://problem/16481284> PLT stops loading on news.google.com

[Darin Adler]

I’m going to land the refactoring half of this,  some time soon, separate from the substantive change since it might be a long time before I can revisit the real behavior change to find out why it interfered with performance testing.

[Darin Adler]

I re-landed the refactoring parts of this patch in bug 131282. Now I’ll make a smaller re-based patch and land it once I can figure out the problem passing PLT.

[Darin Adler]

Created attachment 228704 [details]
Patch

[WebKit Commit Bot]

Attachment 228704 [details] did not pass style-queue:


ERROR: Source/WebCore/html/HTMLPlugInImageElement.cpp:280:  Missing space before {  [whitespace/braces] [5]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexey Proskuryakov]

Comment on attachment 228704 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=228704&action=review

> LayoutTests/ChangeLog:9
> +        * plugins/plugin-remove-readystatechange-expected.txt: Added a blank line; not what this
> +        test is testing, but the different timing of loads results in this blank line.

I think that this is a bug in render tree creation code that this blank line appears; if test flakiness is to be trusted, it's a regression from lazy render tree creation.

Posted some more details into bug 126169, where I attempt to work around the bug.

[Darin Adler]

Created attachment 296864 [details]
Patch

[Darin Adler]

This patch, now rebased, was already reviewed more than two years ago, and passes regression tests. But I fear it might cause the rdar://problem/16481284 problem again, so I am not landing it right now. We want the performance boost from this, so perhaps someone can help me test and fix whatever problem was causing the PLT to get stuck.

[Antti Koivisto]

We have a bunch of old code that tries to eliminate unnecessary white-space-only text renderers (as an optimisation). This code can produce different results depending on exact scope of the render tree update. The scope can be timing sensitive as updates coalesce.

These renderers are visually unobservable but can affect text dumps (and render tree dumps, obviously) by causing blank lines. Sounds like the test is hitting this case. While it would be nice to eliminate the variance (maybe by improving text dump generation) it might be easier to just change the test so it doesn't hit this issue.

[Build Bot]

Comment on attachment 296864 [details]
Patch

Attachment 296864 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2697758

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 296865 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 296864 [details]
Patch

Attachment 296864 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2697752

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 296866 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 296864 [details]
Patch

Attachment 296864 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2697841

New failing tests:
svg/wicd/test-rightsizing-a.xhtml
fast/css/acid2-pixel.html
imported/w3c/web-platform-tests/html/dom/reflection-embedded.html
imported/w3c/web-platform-tests/html/browsers/the-window-object/accessing-other-browsing-contexts/indexed-browsing-contexts-02.html
imported/w3c/web-platform-tests/html/browsers/the-window-object/accessing-other-browsing-contexts/indexed-browsing-contexts-03.html
svg/wicd/test-rightsizing-b.xhtml

[Build Bot]

Created attachment 296867 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 296864 [details]
Patch

Attachment 296864 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2697849

New failing tests:
imported/w3c/web-platform-tests/html/browsers/the-window-object/accessing-other-browsing-contexts/indexed-browsing-contexts-03.html
imported/w3c/web-platform-tests/html/dom/reflection-embedded.html
fast/css/acid2-pixel.html
svg/wicd/test-rightsizing-a.xhtml
imported/w3c/web-platform-tests/html/browsers/the-window-object/accessing-other-browsing-contexts/indexed-browsing-contexts-02.html

[Build Bot]

Created attachment 296868 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Darin Adler]

(In reply to comment #18)
> We have a bunch of old code that tries to eliminate unnecessary
> white-space-only text renderers (as an optimisation). This code can produce
> different results depending on exact scope of the render tree update. The
> scope can be timing sensitive as updates coalesce.
> 
> These renderers are visually unobservable but can affect text dumps (and
> render tree dumps, obviously) by causing blank lines. Sounds like the test
> is hitting this case. While it would be nice to eliminate the variance
> (maybe by improving text dump generation) it might be easier to just change
> the test so it doesn't hit this issue.

I didn’t think the problem with the whitespace was still happening. I didn’t see that problem in my local testing and Alexey claimed to have worked around it long ago in bug 126169 back in 2014.

The problem that I *thought* was preventing me from being able to land this now in 2016 is that our Apple-internal page load test had failed the last time we tried this, getting stuck on news.google.com.

And I now see that my current patch failed a bunch of other tests on EWS, so first I’ll have to fix whatever is causing those failures before thinking about that page load test problem.

[Andreas Kling]

If you fix this bug and get rid of its companion updateStyleIfNeeded() in Document::finishedParsing(), you could get a ~3x speedup on PerformanceTests/Parser/html-parser.html

Then we'd really be cooking :)

[Darin Adler]

Committed r219824: <http://trac.webkit.org/changeset/219824>

[Andreas Kling]

Looks like the test failures are caused by DRT/WTR prematurely dumping the render tree.

It's a long chain of calls, but the dumping is essentially driven by FrameLoaderClient::dispatchDidFinishLoad(), invoked by FrameLoader::checkLoadCompleteForThisFrame().

This gets called basically every time any resource finished loading or has an error, and has no protection against the delay-load-event counter on Document. FrameLoader decides that everything it cares about is finished, and so sounds the alarm.

Simply adding this to the top of FrameLoader::checkLoadCompletedForThisFrame() should fix the failing tests:

    if (m_frame.document()->isDelayingLoadEvent())
        return;

But.. if we look at the start of FrameLoader::checkCompleted(), this is but one of many checks that we might care about in checkLoadCompleteForThisFrame(), too.

[Darin Adler]

Wow, thanks! I will look into that.

[Darin Adler]

Created attachment 316420 [details]
Patch

[Build Bot]

Comment on attachment 316420 [details]
Patch

Attachment 316420 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4188000

New failing tests:
webgl/1.0.2/conformance/extensions/oes-texture-float-with-image-data.html
webarchive/loading/video-in-webarchive.html
webgl/1.0.2/conformance/textures/tex-image-and-sub-image-2d-with-image-data-rgba5551.html
webgl/1.0.2/conformance/textures/tex-image-and-sub-image-2d-with-image-data-rgba4444.html
webgl/1.0.2/conformance/context/context-attributes-alpha-depth-stencil-antialias.html
fast/text/international/embed-bidi-style-in-isolate-crash.html
webgl/1.0.2/conformance/textures/copy-tex-image-and-sub-image-2d.html
webgl/1.0.3/conformance/extensions/oes-texture-half-float-with-image-data.html
webgl/1.0.2/conformance/textures/tex-image-and-sub-image-2d-with-image-data-rgb565.html
media/track/track-in-band-duplicate-tracks-when-source-changes.html
webgl/1.0.2/conformance/textures/tex-image-and-sub-image-2d-with-image-data.html
webgl/1.0.3/conformance/extensions/oes-texture-float-linear.html

[Build Bot]

Created attachment 316426 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 316420 [details]
Patch

Attachment 316420 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4188038

New failing tests:
webgl/1.0.2/conformance/extensions/oes-texture-float-with-image-data.html
webarchive/loading/video-in-webarchive.html
webgl/1.0.2/conformance/textures/tex-image-and-sub-image-2d-with-image-data-rgba5551.html
media/event-queue-crash.html
webgl/1.0.2/conformance/textures/tex-image-and-sub-image-2d-with-image-data-rgba4444.html
fast/text/international/embed-bidi-style-in-isolate-crash.html
webgl/1.0.2/conformance/textures/copy-tex-image-and-sub-image-2d.html
webgl/1.0.3/conformance/extensions/oes-texture-half-float-with-image-data.html
webgl/1.0.2/conformance/textures/tex-image-and-sub-image-2d-with-image-data-rgb565.html
media/video-click-dblckick-standalone.html
media/video-element-other-namespace-crash.html
media/track/track-in-band-duplicate-tracks-when-source-changes.html
webgl/1.0.2/conformance/textures/tex-image-and-sub-image-2d-with-image-data.html
webgl/1.0.3/conformance/textures/tex-image-canvas-corruption.html

[Build Bot]

Created attachment 316428 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Build Bot]

Comment on attachment 316420 [details]
Patch

Attachment 316420 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4187969

New failing tests:
webgl/1.0.2/conformance/extensions/oes-texture-float-with-image-data.html
webarchive/loading/video-in-webarchive.html
webgl/1.0.2/conformance/textures/tex-image-and-sub-image-2d-with-image-data-rgba5551.html
webgl/1.0.2/conformance/textures/tex-image-and-sub-image-2d-with-image-data-rgba4444.html
webgl/1.0.2/conformance/context/context-attributes-alpha-depth-stencil-antialias.html
imported/blink/fast/dom/Window/open-window-features-fuzz.html
media/modern-media-controls/media-documents/background-color-and-centering.html
webgl/1.0.2/conformance/textures/copy-tex-image-and-sub-image-2d.html
webgl/1.0.3/conformance/extensions/oes-texture-half-float-with-image-data.html
webgl/1.0.2/conformance/textures/tex-image-and-sub-image-2d-with-image-data-rgb565.html
media/track/track-in-band-duplicate-tracks-when-source-changes.html
fast/text/international/embed-bidi-style-in-isolate-crash.html
webgl/1.0.2/conformance/textures/tex-image-and-sub-image-2d-with-image-data.html
webgl/1.0.3/conformance/extensions/oes-texture-float-linear.html

[Build Bot]

Created attachment 316429 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 316420 [details]
Patch

Attachment 316420 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4188024

New failing tests:
webgl/1.0.2/conformance/extensions/oes-texture-float-with-image-data.html
webarchive/loading/video-in-webarchive.html
webgl/1.0.2/conformance/textures/tex-image-and-sub-image-2d-with-image-data-rgba4444.html
webgl/1.0.2/conformance/textures/tex-image-and-sub-image-2d-with-image-data-rgba5551.html
webgl/1.0.2/conformance/context/context-attributes-alpha-depth-stencil-antialias.html
fast/text/international/embed-bidi-style-in-isolate-crash.html
webgl/1.0.2/conformance/textures/copy-tex-image-and-sub-image-2d.html
webgl/1.0.3/conformance/extensions/oes-texture-half-float-with-image-data.html
webgl/1.0.2/conformance/textures/tex-image-and-sub-image-2d-with-image-data-rgb565.html
media/video-click-dblckick-standalone.html
media/video-element-other-namespace-crash.html
media/track/track-in-band-duplicate-tracks-when-source-changes.html
webgl/1.0.2/conformance/textures/tex-image-and-sub-image-2d-with-image-data.html
webgl/1.0.3/conformance/extensions/oes-texture-float-linear.html

[Build Bot]

Created attachment 316430 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Darin Adler]

For those of you following along (I’m looking at you Mr. Kling), now that I added the additional check in FrameLoader::checkLoadCompletedForThisFrame, we now have a few different categories of failing tests:

Regressions: Unexpected timeouts
  media/track/track-in-band-duplicate-tracks-when-source-changes.html [ Timeout ]
  webgl/1.0.2/conformance/context/context-attributes-alpha-depth-stencil-antialias.html [ Timeout ]
  webgl/1.0.2/conformance/extensions/oes-texture-float-with-image-data.html [ Timeout ]
  webgl/1.0.2/conformance/textures/copy-tex-image-and-sub-image-2d.html [ Timeout ]
  webgl/1.0.2/conformance/textures/tex-image-and-sub-image-2d-with-image-data-rgb565.html [ Timeout ]
  webgl/1.0.2/conformance/textures/tex-image-and-sub-image-2d-with-image-data-rgba4444.html [ Timeout ]
  webgl/1.0.2/conformance/textures/tex-image-and-sub-image-2d-with-image-data-rgba5551.html [ Timeout ]
  webgl/1.0.2/conformance/textures/tex-image-and-sub-image-2d-with-image-data.html [ Timeout ]
  webgl/1.0.3/conformance/extensions/oes-texture-float-linear.html [ Timeout ]
  webgl/1.0.3/conformance/extensions/oes-texture-half-float-with-image-data.html [ Timeout ]

^^^ Without actually studying anything here and diagnosing, here is my guess: Given that my last change was to make FrameLoader::checkLoadCompletedForThisFrame decide the load is not completed in some cases, it’s likely that we need to pair that with a change that guarantees FrameLoader::checkLoadCompletedForThisFrame is called once the document is no longer delaying the load event.

Regressions: Unexpected text-only failures
  webarchive/loading/video-in-webarchive.html [ Failure ]

^^^ The issue here seems to different ordering of "load completed" callbacks; to me it seems likely the new behavior is correct and my patch just needs to include new expected results for this test.

Other failures seem likely unrelated to the patch; not sure what to do about those. Need some more "gardening" it seems.

[Darin Adler]

(In reply to Darin Adler from comment #41)
> Given that my last change was to make
> FrameLoader::checkLoadCompletedForThisFrame decide the load is not completed
> in some cases, it’s likely that we need to pair that with a change that
> guarantees FrameLoader::checkLoadCompletedForThisFrame is called once the
> document is no longer delaying the load event.

Document::loadEventDelayTimerFired does call FrameLoader::checkLoadComplete, which calls FrameLoader::checkLoadCompleteForThisFrame on all the frames, not just the one with the document.

Some possible (unlikely) holes in this:

1) If the document's frame get set to some other frame or to null, we might never call FrameLoader::checkLoadComplete.

2) If the frame's page gets set to null at just the wrong time, FrameLoader::checkLoadComplete might decide not to do anything.

Given that my above guess doesn’t seem like it can be exactly right. Need to debug probably rather than just studying the code.

Another possibility is that isDelayingLoadEvent should block when in FrameStateCommittedPage and FrameStateComplete but not FrameStateProvisional. Unlikely to actually be the issue in this case, but it seems in inappropriate for isDelayingLoadEvent to prevent a frame from moving from provisional to committed state.

[Andreas Kling]

>I’m looking at you Mr. Kling

Oh jeez, guess I better hop to it!

>2) If the frame's page gets set to null at just the wrong time, FrameLoader::checkLoadComplete might decide not to do anything.

This is the correct guess! Some frames are now unloaded before they get a chance to fire a load event in their documents, and so the Document::frame() is null when we get to decrementLoadEventDelayCount().

Here's a backtrace for Document::detachFromFrame() when running webgl/1.0.3/conformance/extensions/oes-texture-float-linear.html

WebCore::Document::detachFromFrame() <--
WebCore::Document::prepareForDestruction()
WebCore::Frame::setView(WTF::RefPtr<WebCore::FrameView>&&)
WebCore::FrameLoader::closeAndRemoveChild(WebCore::Frame&)
WebCore::FrameLoader::detachFromParent()
0x10ffab7c6 WebCore::FrameLoader::frameDetached()
WebCore::HTMLFrameOwnerElement::disconnectContentFrame()
WebCore::disconnectSubframes(WebCore::ContainerNode&, WebCore::SubframeDisconnectPolicy)
WebCore::willRemoveChildren(WebCore::ContainerNode&)
WebCore::ContainerNode::removeChildren()
WebCore::replaceChildrenWithFragment(WebCore::ContainerNode&, WTF::Ref<WebCore::DocumentFragment>&&)
WebCore::Element::setInnerHTML(WTF::String const&)
WebCore::setJSElementInnerHTML(JSC::ExecState*, long long, long long)
JSC::callCustomSetter(JSC::ExecState*, JSC::JSValue, bool, JSC::JSObject*, JSC::JSValue, JSC::JSValue)
JSC::JSObject::putInlineSlow(JSC::ExecState*, JSC::PropertyName, JSC::JSValue, JSC::PutPropertySlot&)
llint_slow_path_put_by_id
llint_entry
vmEntryToJavaScript
JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*)
JSC::Interpreter::executeProgram(JSC::SourceCode const&, JSC::ExecState*, JSC::JSObject*)
JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&)
WebCore::ScriptController::evaluateInWorld(WebCore::ScriptSourceCode const&, WebCore::DOMWrapperWorld&, WebCore::ExceptionDetails*)
WebCore::ScriptElement::executeClassicScript(WebCore::ScriptSourceCode const&)
WebCore::LoadableClassicScript::execute(WebCore::ScriptElement&)
WebCore::ScriptElement::executeScriptAndDispatchEvent(WebCore::LoadableScript&)
WebCore::ScriptRunner::timerFired()
WebCore::ThreadTimers::sharedTimerFiredInternal()
WebCore::timerFired(__CFRunLoopTimer*, void*)

I think what happens is that a <script> is inserted somewhere via modifying innerHTML. ScriptRunner sets up a zero-timer and executes the script in the callback.
At the end of ScriptRunner::timerFired(), we call decrementLoadEventDelayCount(), but by then it's too late, the script made the document detach from the frame. :/

[Andreas Kling]

Something is off here though, looking at the webgl test harness code:
LayoutTests/webgl/1.0.3/resources/webkit-webgl-test-harness.js

It appears to me that this test uses testRunner.waitUntilDone() / testRunner.notifyDone() and so shouldn't depend on the whimsy of our load event.

I think maybe it's something else that's broken here.

[Darin Adler]

The problem is caused by the increment/decrementLoadEventDelayCount in ScriptRunner. By the time decrementLoadEventDelayCount is called, the document's frame is already null.

[Darin Adler]

That’s because the script overwrites the frame’s content with a pass/fail result.

I have a fix, but it’s pretty peculiar.

[Darin Adler]

Created attachment 316518 [details]
Patch

[Build Bot]

Comment on attachment 316518 [details]
Patch

Attachment 316518 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4194563

New failing tests:
webarchive/loading/video-in-webarchive.html
media/event-queue-crash.html
fast/text/international/embed-bidi-style-in-isolate-crash.html
media/video-click-dblckick-standalone.html
media/video-element-other-namespace-crash.html
media/track/track-in-band-duplicate-tracks-when-source-changes.html

[Build Bot]

Created attachment 316524 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Build Bot]

Comment on attachment 316518 [details]
Patch

Attachment 316518 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4194590

New failing tests:
media/track/track-in-band-duplicate-tracks-when-source-changes.html
webarchive/loading/video-in-webarchive.html
imported/blink/fast/dom/Window/open-window-features-fuzz.html
fast/text/international/embed-bidi-style-in-isolate-crash.html

[Build Bot]

Created attachment 316525 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 316518 [details]
Patch

Attachment 316518 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4194996

New failing tests:
media/track/track-in-band-duplicate-tracks-when-source-changes.html
webarchive/loading/video-in-webarchive.html
fast/text/international/embed-bidi-style-in-isolate-crash.html

[Build Bot]

Created attachment 316531 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 316518 [details]
Patch

Attachment 316518 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4195015

New failing tests:
media/video-click-dblckick-standalone.html
media/video-element-other-namespace-crash.html
media/track/track-in-band-duplicate-tracks-when-source-changes.html
webarchive/loading/video-in-webarchive.html
fast/text/international/embed-bidi-style-in-isolate-crash.html

[Build Bot]

Created attachment 316532 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Darin Adler]

Created attachment 316731 [details]
Patch

[Darin Adler]

To fix one of the failing tests, I had to make a change to HTMLMediaElement. I made it after reading the HTML specification text; the fix makes us match the specification. But I do want some media loading experts to look, not so much because I doubt the correctness of the change in the patch, but because the FIXME I am adding points to some other similar changes we might want to make.

[Darin Adler]

Maybe we should add some new tests for this media loading fix.

[Build Bot]

Comment on attachment 316731 [details]
Patch

Attachment 316731 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4222150

New failing tests:
imported/blink/fast/dom/Window/open-window-features-fuzz.html
fast/text/international/embed-bidi-style-in-isolate-crash.html

[Build Bot]

Created attachment 316733 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 316731 [details]
Patch

Attachment 316731 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4222160

New failing tests:
media/video-click-dblckick-standalone.html
media/video-element-other-namespace-crash.html
media/event-queue-crash.html
fast/text/international/embed-bidi-style-in-isolate-crash.html

[Build Bot]

Created attachment 316734 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Darin Adler]

Created attachment 316735 [details]
Patch

[Build Bot]

Comment on attachment 316735 [details]
Patch

Attachment 316735 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4224154

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 316736 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 316735 [details]
Patch

Attachment 316735 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4224202

New failing tests:
contentfiltering/allow-after-will-send-request.html
contentfiltering/allow-after-add-data.html
http/tests/misc/location-replace-crossdomain.html
http/tests/loading/server-redirect-for-provisional-load-caching.html
contentfiltering/allow-never.html
contentfiltering/allow-after-finished-adding-data.html
http/tests/navigation/postredirect-frames.html
http/tests/security/contentSecurityPolicy/block-all-mixed-content/insecure-css-in-iframe-report-only.html
fast/events/before-unload-remove-and-add-subframe.html
contentfiltering/block-after-will-send-request-then-allow-unblock.html
http/tests/loading/authentication-after-redirect-stores-wrong-credentials/authentication-after-redirect-stores-wrong-credentials.html
contentfiltering/block-after-response.html
contentfiltering/block-after-response-then-allow-unblock.html
http/tests/security/frame-loading-via-document-write.html
contentfiltering/block-after-add-data.html
fast/events/before-unload-adopt-within-subframes.html
contentfiltering/block-after-add-data-then-allow-unblock.html
contentfiltering/block-after-will-send-request.html
contentfiltering/block-after-finished-adding-data.html
contentfiltering/block-never.html
contentfiltering/block-after-finished-adding-data-then-allow-unblock.html
contentfiltering/allow-after-response.html

[Build Bot]

Created attachment 316737 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Darin Adler]

Created attachment 316741 [details]
Patch

[Build Bot]

Comment on attachment 316741 [details]
Patch

Attachment 316741 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4225040

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 316743 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Darin Adler]

Created attachment 316748 [details]
Patch

[Darin Adler]

After years of trying and lots of craziness, looks like this is ready for review again.

Antti reviewed the original applet/embed/frame/iframe/object element loading change, and there is a little more to the patch now because of bugs I had to fix in five or six regression tests, in a small part of the load event logic in document, document loader, and frame loader, in the "when to dump" logic in WebKitTestRunner, and in the load event delay machinery in one small place in the media element.

But patch still didn’t get giant. And this one is likely to pass all the EWS tests.

[Darin Adler]

Created attachment 316750 [details]
Patch

[Darin Adler]

Comment on attachment 316750 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=316750&action=review

Looks like the change log is missing quite a few of the changes. I’ll likely upload a new patch later with a better change log.

> Source/WebCore/html/HTMLPlugInImageElement.cpp:295
> +        document().incrementLoadEventDelayCount();

Style-wise should probably be newDocument rather than document(), although I don’t see how that can matter given we assert they are the same. My patch predated the addition of the newDocument argument.

[Antti Koivisto]

Comment on attachment 316750 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=316750&action=review

r=me, nice

> Source/WebCore/dom/Document.cpp:6228
> +    // FIXME: Should the call to FrameLoader::checkLoadComplete be moved inside Document::checkCompleted?
> +    // FIXME: Should this also call DocumentLoader::checkLoadComplete?
> +    // FIXME: Not obvious why checkCompleted needs to go first. The order these are called is
> +    // visible to WebKit clients, but it's more like a race than a well-defined relationship.

This logic should be cleaned up at some point. It feels as if the original intent of the differnt check/loadCompleted/isLoading functions has been lost at some point and they are now being called without rhyme or reason.

[Darin Adler]

(In reply to Antti Koivisto from comment #75)
> This logic should be cleaned up at some point. It feels as if the original
> intent of the differnt check/loadCompleted/isLoading functions has been lost
> at some point and they are now being called without rhyme or reason.

My thought exactly. Basically there is one concept here from KHTMLPart (checkComplete) and one from the Objective-C WebKit framework (checkLoadComplete). We never reconciled the two when we put them together in the same framework.

[Darin Adler]

I have this ready to land now with a better ChangeLog. Will land after I confirm that the EWS succeeds.

[Build Bot]

Comment on attachment 316750 [details]
Patch

Attachment 316750 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4225801

New failing tests:
fast/images/large-size-image-crash.html

[Build Bot]

Created attachment 316751 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Darin Adler]

Committed r220052: <http://trac.webkit.org/changeset/220052>

[Andreas Kling]

Great work! 32% speedup on PerformanceTests/Parser/HTML5-8266-FullRender.html :)

https://perf.webkit.org/v3/#/charts?since=1500879248694&zoom=(1501396022165.3025-1501482250189.8582)&paneList=((18-311-null-null-(5-2.5-500)))

[Sam Weinig]

Wow!

Great! 1-3% page load progression is observed.

[Alexey Proskuryakov]

This caused bug 174988. I'm somewhat hesitant to roll back right away, but I may have to, as there quite a few new failures over the weekend, and we need to make bots green soon.

[Darin Adler]

I suggest skipping the test to make the bots green. I absolutely will fix this bug and reenable the test, and I think a rollout is unnecessary.

[Matt Lewis]

This caused bug: https://bugs.webkit.org/show_bug.cgi?id=175107

[Alexey Proskuryakov]

Another regression: bug 175329.

[Radar WebKit Bug Importer]

<rdar://problem/34191442>