From 8f3d4f59182a1df4d7ca548bdae37e56a0ad263e Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Tue, 12 Mar 2019 10:32:04 -0700 Subject: [PATCH] DocumentLoader: commit navigation synchronously. Attempt #2 We used to commit navigation after receiving the first byte of document response. This CL moves commit earlier, synchronously done from CommitNavigation call. The change should not be web-observable, but some internal assumptions may have been affected. Test changes: - ReplacingDocumentLoaderFiresLoadEvent was testing the old behavior, which is not applicable anymore. - MultiChunkWithReentrancy now uses a different method to trigger reentrancy (pdf plugin), since we no longer commit after first byte. - backdrop-object.html and anchor-change-href.svg relied on test finishing late enough, now they wait for onload to eliminate a race. - use-property-synchronization-crash.html now reports an error message synchronously and therefore has JS stack and a line number. - setting-allowpaymentrequest-timing.https.sub.html has a race as explained here [1], and now fails even without site isolation. This corresponds to the step 8.b from the doc linked to the bug. Difference from attempt #1 (https://chromium-review.googlesource.com/c/1399447): - PluginDocumentParser and MediaDocumentParser early return if not parsing before accessing GetDocument. This is because DocumentLoader calls Finish() even after parser was stopped/detached. For example in Document::Abort we cancel parsing, but committed DocumentLoader might be still receiving data. We should ideally clean up all calls into parser, there are numerous TODOs for that. - pageload-image-in-popup.html relies on small image being parsed in the same task as navigation commit. Using onload seems to fix the issue. - touch-handler-iframe-plugin-assert.js hopes that onload for about:blank happens after test has finished, which is racy now. [1] https://bugs.chromium.org/p/chromium/issues/detail?id=819800#c6 Bug: 855189, 937639, 836242, 937358 Change-Id: I65048a27e6d249a608d4eb61e5c882292386026e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1506663 Commit-Queue: Dmitry Gozman Reviewed-by: Daniel Cheng Reviewed-by: Arthur Sonzogni Cr-Commit-Position: refs/heads/master@{#639992} --- .../read-media/pageload-image-in-popup.html | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/html/browsers/browsing-the-web/read-media/pageload-image-in-popup.html b/html/browsers/browsing-the-web/read-media/pageload-image-in-popup.html index 4d811199b146ec..e9284824f44f2d 100644 --- a/html/browsers/browsing-the-web/read-media/pageload-image-in-popup.html +++ b/html/browsers/browsing-the-web/read-media/pageload-image-in-popup.html @@ -13,18 +13,7 @@ var t = async_test("The document for a standalone media file should have one child in the body."); var imgwin = window.open('/images/blue.png'); - // imgwin.onload doesn't work, check popup's URL to see if the loading of - // the image and creation of image document is finished. - function checkURL() { - if (imgwin.location.href.indexOf('blue.png') == -1) { - step_timeout(checkURL, 100); - return; - } - t.step(frameLoaded); - } - checkURL(); - - function frameLoaded() { + imgwin.onload = t.step_func_done(function() { assert_equals(imgwin.opener, window); assert_equals(imgwin.document.contentType, "image/png"); var imgwinChildren = imgwin.document.body.childNodes; @@ -33,8 +22,7 @@ assert_equals(imgwinChildren[0].namespaceURI, "http://www.w3.org/1999/xhtml", "Only child of body must be an HTML element"); imgwin.close(); - t.done(); - } + });