New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 855033 link

Starred by 1 user
Status: Fixed
Owner:
arthurso...@chromium.org
Closed: Jun 2018
Cc:
japhet@chromium.org
clamy@chromium.org
dgozman@chromium.org
dcheng@chromium.org
█ nasko@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

DidStopLoading() not always fired when a download causes the parser of a loading document to stop.

Project Member Reported by arthurso...@chromium.org, Jun 21 2018 
When does it happens:
1) URLLoaderClient::OnComplete() is received before the response body data pipe is fully read.
   It can be achieved by making the DocumentParser slow.
2) The parser of the current document is cancelled by a new navigation.
   The bug is that CheckCompleted() is not called on the the current document at this step.
3) The navigation is a download, so it is dropped.
4) DidStopLoading() is never fired.

Regression test: https://chromium-review.googlesource.com/c/chromium/src/+/1109974

+CC dcheng@ dgozman@ japhet@ clamy@ FYI.
I finally find a way to make a regression test for the fix you are reviewing.
I am not sure this test deserves to be committed thought.

This is fixed by:
https://chromium-review.googlesource.com/c/chromium/src/+/1087227 or
https://chromium-review.googlesource.com/c/chromium/src/+/1107808

Comment 1 by arthurso...@chromium.org, Jun 27 2018

Status: Fixed (was: Started)
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c0327d09b6e4444caca64f3bd16ec0bd20034049

commit c0327d09b6e4444caca64f3bd16ec0bd20034049
Author: Arthur Sonzogni <arthursonzogni@chromium.org>
Date: Mon Jun 25 10:40:27 2018

Call Document::CheckCompleted() after CancelParsing..

Document::CancelParsing() may cause Document::ShouldComplete() to return
true. Document::CheckCompleted() needs to be called so that its
DocumentLoader is marked to have finished its load.

A lot of tests don't expect DidStopLoading() and DidStartLoading() to be
fired when a new DocumentLoader starts and replaces the previous one.
In FrameLoader::StartLoad(), Document::CancelParsing() is moved after
the creation of the new provisional DocumentLoader so that the
FrameLoader isn't temporarily without any non loading Document. If that
happened, DidStopLoading() would be fired.

There was also one small bug with site isolation. The condition
|have_to_check_if_parent_is_completed| in
HTMLFrameOwnerElement::DisconnectContentFrame() was too strict. The
parent document may have completed its load, but not have sent
DidStopLoading yet because its childs were still loading.

Note: I am working on https://crbug.com/831155. Some tests that are
relying on how things are scheduled in Blink are failing. This CL makes
DownloadTest.TestMultipleDownloadsRequests to keep working. Here, while
the current document is loading, a new navigation happens and cancels
the parser of the current document without calling
Document::CheckCompleted() on it. Then, it turns out the navigation is a
download. The navigation is dropped. In the end the main frame doesn't
know its iframe has finished loading and the test fails.

Bug: 831155

Change-Id: Ib2de108e25991349b44ec4071fe61d57ca6e85a1
Reviewed-on: https://chromium-review.googlesource.com/1107808
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570002}
[modify] https://crrev.com/c0327d09b6e4444caca64f3bd16ec0bd20034049/content/browser/download/download_browsertest.cc
[modify] https://crrev.com/c0327d09b6e4444caca64f3bd16ec0bd20034049/third_party/blink/renderer/core/html/html_frame_owner_element.cc
[modify] https://crrev.com/c0327d09b6e4444caca64f3bd16ec0bd20034049/third_party/blink/renderer/core/loader/frame_loader.cc
Project Member

Comment 2 by bugdroid1@chromium.org, Jun 28 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ece30c64519a6c089adf7befe5fe4a74190c853d

commit ece30c64519a6c089adf7befe5fe4a74190c853d
Author: Arthur Sonzogni <arthursonzogni@chromium.org>
Date: Thu Jun 28 09:57:29 2018

FrameLoader. Add test for  bug 855033 .

The bug happens when the current document data has been loaded, but the
parser is still processing it. Then a new navigation happens and cancels
the current document's parser. The issue is that the Document was not
marked to be 'loaded' at this step. Then if the navigation turns to be
download, it is dropped and there is no more chance for the frame to be
marked as 'loaded'.

Bug:  855033 
Change-Id: I572470dd1ad53d9905b3604f6a91ce6f4bd53952
Reviewed-on: https://chromium-review.googlesource.com/1109974
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Reviewed-by: Camille Lamy <clamy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#571061}
[modify] https://crrev.com/ece30c64519a6c089adf7befe5fe4a74190c853d/content/browser/browser_side_navigation_browsertest.cc
►

Sign in to add a comment