|
Description
Darin Adler
2014-03-23 12:19:16 PDT
Comment hidden (obsolete)
Created attachment 227613 [details]
Patch
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.
Style bot doesn’t like the way I am formatting lambdas. Do we need to fix it? 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." Created attachment 227616 [details]
Patch
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. 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.
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? (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. 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 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. 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. Created attachment 228704 [details]
Patch
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.
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. Created attachment 296864 [details]
Patch
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. 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. 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. 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
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. 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
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 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
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 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
(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. 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 :) Committed r219824: <http://trac.webkit.org/changeset/219824> 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.
Wow, thanks! I will look into that. Created attachment 316420 [details]
Patch
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 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
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 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
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 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
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 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
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. (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. >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. :/ 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. The problem is caused by the increment/decrementLoadEventDelayCount in ScriptRunner. By the time decrementLoadEventDelayCount is called, the document's frame is already null. That’s because the script overwrites the frame’s content with a pass/fail result. I have a fix, but it’s pretty peculiar. Created attachment 316518 [details]
Patch
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 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
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 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
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 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
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 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
Created attachment 316731 [details]
Patch
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. Maybe we should add some new tests for this media loading fix. 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 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
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 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
Created attachment 316735 [details]
Patch
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. 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
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 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
Created attachment 316741 [details]
Patch
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. 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
Created attachment 316748 [details]
Patch
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. Created attachment 316750 [details]
Patch
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. 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. (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. I have this ready to land now with a better ChangeLog. Will land after I confirm that the EWS succeeds. 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 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
Committed r220052: <http://trac.webkit.org/changeset/220052> 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))) Wow! Great! 1-3% page load progression is observed. 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. 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. This caused bug: https://bugs.webkit.org/show_bug.cgi?id=175107 Another regression: bug 175329. |