Issue 1481063002: WIP: DO NOT COMMIT.

Issue 1481063002: WIP: DO NOT COMMIT. (Closed)

Created:
5 years ago by Mike West

Modified:
5 years ago

Reviewers:
gsennton

CC:
chromium-reviews, blink-reviews, loading-reviews_chromium.org, tyoshino+watch_chromium.org, Nate Chapin, gavinp+loader_chromium.org

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

WIP: DO NOT COMMIT.

Patch Set 1 #

Created: 5 years ago

Download [raw] [tar.bz2]

		Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+8 lines, -4 lines)			Patch
	M	third_party/WebKit/Source/core/loader/FrameLoader.cpp	View		1 chunk	+8 lines, -4 lines	0 comments	Download

Messages

Total messages: 5 (1 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

gsennton

Right, so the browser test WebContentsImplBrowserTest.DidStopLoadingDetailsWithPending that is failing (for me locally running Linux the ...

5 years ago (2015-12-01 10:33:42 UTC) #2

gsennton

On 2015/12/01 10:33:42, gsennton wrote: > Right, so the browser test > WebContentsImplBrowserTest.DidStopLoadingDetailsWithPending that is ...

5 years ago (2015-12-01 12:41:18 UTC) #3

On 2015/12/01 10:33:42, gsennton wrote:
> Right, so the browser test
> WebContentsImplBrowserTest.DidStopLoadingDetailsWithPending that is failing
(for
> me locally running Linux the test is not failing consistently but is flaky),
was
> already flaky before the patch (crbug.com/298193). Can this be ignored? (it is
> disabled on Windows and Linux).
> 
> The only other test that fails is
> fast/loader/subframe-removes-itself.html
> 
> With this patch it prints 
> 
> 
> frame "<!--framePath //<!--frame0-->-->" - didStartProvisionalLoadForFrame
> main frame - didFinishDocumentLoadForFrame
> frame "<!--framePath //<!--frame0-->-->" - didCommitLoadForFrame
> frame "<!--framePath //<!--frame0-->-->" - didFailLoadWithError
> frame "<!--framePath //<!--frame0-->-->" - didFailLoadWithError
> main frame - didHandleOnloadEventsForFrame
> main frame - didFinishLoadForFrame
> This tests a subframe that removes itself to make sure a delegate message to
> indicate the interrupted load fires.
> 
> 
> when it should be printing
> 
> frame "<!--framePath //<!--frame0-->-->" - didStartProvisionalLoadForFrame
> main frame - didFinishDocumentLoadForFrame
> frame "<!--framePath //<!--frame0-->-->" - didCommitLoadForFrame
> frame "<!--framePath //<!--frame0-->-->" - didFailLoadWithError
> main frame - didHandleOnloadEventsForFrame
> main frame - didFinishLoadForFrame
> This tests a subframe that removes itself to make sure a delegate message to
> indicate the interrupted load fires.
> 
> 
> I.e. we receive one extra didFailLoadWithError for the subframe.

Aha! Found el problemo:

In this test we are calling DocumentLoader::mainReceivedError twice (for the
committed loader) for some reason, and in mainReceivedError we set m_state to
MainResourceDone unconditionally, i.e. even if we have set m_state =
SentDidFinishLoad in an earlier call to FrameLoader::receivedMainResourceError
we still reset m_state here. This means that in every call to
FrameLoader::receivedMainResourceError from the committed document loader, we
will enter the if-statement

if (!m_documentLoader->sentDidFinishLoad()) {
    client()->dispatchDidFailLoad(error, historyCommitType);
    m_documentLoader->setSentDidFinishLoad();
}

and thus dispatch didFail again. Before this patch this worked because the
if-statement would check m_frame->isLoading() which would then be set to false
by ProgressTracker::ProgressCompleted.

So unless we want to disallow several calls to mainReceivedError I suggest we
simply make the m_state assignment conditional. I.e.
we change

m_state = MainResourceDone;

to

if (m_state < MainResourceDone) m_state = MainResourceDone;

Mike West

On 2015/12/01 at 12:41:18, gsennton wrote: > On 2015/12/01 10:33:42, gsennton wrote: > > Right, ...

5 years ago (2015-12-01 12:47:26 UTC) #4

On 2015/12/01 at 12:41:18, gsennton wrote:
> On 2015/12/01 10:33:42, gsennton wrote:
> > Right, so the browser test
> > WebContentsImplBrowserTest.DidStopLoadingDetailsWithPending that is failing
(for
> > me locally running Linux the test is not failing consistently but is flaky),
was
> > already flaky before the patch (crbug.com/298193). Can this be ignored? (it
is
> > disabled on Windows and Linux).
> > 
> > The only other test that fails is
> > fast/loader/subframe-removes-itself.html
> > 
> > With this patch it prints 
> > 
> > 
> > frame "<!--framePath //<!--frame0-->-->" - didStartProvisionalLoadForFrame
> > main frame - didFinishDocumentLoadForFrame
> > frame "<!--framePath //<!--frame0-->-->" - didCommitLoadForFrame
> > frame "<!--framePath //<!--frame0-->-->" - didFailLoadWithError
> > frame "<!--framePath //<!--frame0-->-->" - didFailLoadWithError
> > main frame - didHandleOnloadEventsForFrame
> > main frame - didFinishLoadForFrame
> > This tests a subframe that removes itself to make sure a delegate message to
> > indicate the interrupted load fires.
> > 
> > 
> > when it should be printing
> > 
> > frame "<!--framePath //<!--frame0-->-->" - didStartProvisionalLoadForFrame
> > main frame - didFinishDocumentLoadForFrame
> > frame "<!--framePath //<!--frame0-->-->" - didCommitLoadForFrame
> > frame "<!--framePath //<!--frame0-->-->" - didFailLoadWithError
> > main frame - didHandleOnloadEventsForFrame
> > main frame - didFinishLoadForFrame
> > This tests a subframe that removes itself to make sure a delegate message to
> > indicate the interrupted load fires.
> > 
> > 
> > I.e. we receive one extra didFailLoadWithError for the subframe.
> 
> Aha! Found el problemo:
> 
> In this test we are calling DocumentLoader::mainReceivedError twice (for the
committed loader) for some reason, and in mainReceivedError we set m_state to
MainResourceDone unconditionally, i.e. even if we have set m_state =
SentDidFinishLoad in an earlier call to FrameLoader::receivedMainResourceError
we still reset m_state here. This means that in every call to
FrameLoader::receivedMainResourceError from the committed document loader, we
will enter the if-statement
> 
> if (!m_documentLoader->sentDidFinishLoad()) {
>     client()->dispatchDidFailLoad(error, historyCommitType);
>     m_documentLoader->setSentDidFinishLoad();
> }
> 
> and thus dispatch didFail again. Before this patch this worked because the
if-statement would check m_frame->isLoading() which would then be set to false
by ProgressTracker::ProgressCompleted.
> 
> So unless we want to disallow several calls to mainReceivedError I suggest we
simply make the m_state assignment conditional. I.e.
> we change
> 
> m_state = MainResourceDone;
> 
> to
> 
> if (m_state < MainResourceDone) m_state = MainResourceDone;

I think that makes a lot of sense. Try sticking it into your patch and running
with it?

gsennton

5 years ago (2015-12-01 13:25:51 UTC) #5

> I think that makes a lot of sense. Try sticking it into your patch and running
> with it?

Will do (it does pass fast/loader/subframe-removes-itself.html locally).

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages