Don't stop the documentLoader on navigations.

Don't stop the documentLoader on navigations.

If the new navigation will be canceled by a Browser-side throttle we
don't want to cancel the currently loading document.

BUG=308257, 378716
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175269

[mkosiba (inactive), 2014-03-04 16:30:29]

Hey! This is me reviving the work I started with
https://codereview.chromium.org/131363006/.

What I'm trying to achieve is to reduce the impact of navigating to a URL that
immediately gets cancelled browser-side.

The hard bit here is guesstimating the implications of moving the
didStart/FinishLoad and didStartProvisionalLoad callback timings. I'm assuming
that stuff's fine as long as tests pass, but...

Anyway - please take a look and tell me whether you think such an approach makes
sense.
A cleaner approach would be to introduce a pre-provisional state where we figure
out whether the request is going to die immediately or not (so essentially
encoding the extra state from m_shouldCallCheckCompleted in m_state). I wanted
to get some initial feedback on you before I go that way though.

[Nate Chapin, 2014-03-10 16:12:44]

Thanks for coming back to this!

https://codereview.chromium.org/181493007/diff/20001/Source/core/loader/Frame...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/181493007/diff/20001/Source/core/loader/Frame...
Source/core/loader/FrameLoader.cpp:624: void
FrameLoader::checkCurrentDocumentLoaderNeedsStop()
If this function is only called once, let's merge it into responseReceived()
above.

https://codereview.chromium.org/181493007/diff/20001/Source/core/loader/Frame...
Source/core/loader/FrameLoader.cpp:637: m_progressTracker->progressStarted();
It doesn't seem right to wait to send a progress tracker start notification
until a response is received.

https://codereview.chromium.org/181493007/diff/20001/Source/core/loader/Frame...
Source/core/loader/FrameLoader.cpp:962: if (m_currentLoaderNeedsStop)
Why is this needed?

https://codereview.chromium.org/181493007/diff/20001/Source/core/loader/Frame...
Source/core/loader/FrameLoader.cpp:1330: if (m_provisionalDocumentLoader)
Merge these 2 ifs? Or can stopLoading() null out the provisional document
loader?

https://codereview.chromium.org/181493007/diff/20001/Source/core/loader/Frame...
File Source/core/loader/FrameLoader.h (right):

https://codereview.chromium.org/181493007/diff/20001/Source/core/loader/Frame...
Source/core/loader/FrameLoader.h:187: void onResponseReceived();
Nit: responseReceived(), to match the name on DocumentLoader.

https://codereview.chromium.org/181493007/diff/20001/Source/core/loader/Frame...
Source/core/loader/FrameLoader.h:271: bool m_currentLoaderNeedsStop;
currentLoader isn't a particularly clear name to me. I'd favor either changing
its name to match one of the DocumentLoaders, or a comment explaining what
exactly it means.

Based on how it's used, it appears to be true when a provisional loader receives
a response, and a committed loader is in progress?

[mkosiba (inactive), 2014-03-11 23:24:07]

https://codereview.chromium.org/181493007/diff/20001/Source/core/loader/Frame...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/181493007/diff/20001/Source/core/loader/Frame...
Source/core/loader/FrameLoader.cpp:624: void
FrameLoader::checkCurrentDocumentLoaderNeedsStop()
On 2014/03/10 16:12:44, Nate Chapin wrote:
> If this function is only called once, let's merge it into responseReceived()
> above.

sure, I was keeping it separate because I was experimenting on calling it from
other places too.

https://codereview.chromium.org/181493007/diff/20001/Source/core/loader/Frame...
Source/core/loader/FrameLoader.cpp:637: m_progressTracker->progressStarted();
On 2014/03/10 16:12:44, Nate Chapin wrote:
> It doesn't seem right to wait to send a progress tracker start notification
> until a response is received.

this is the meat of the change. The problem is that when you call
loadWithNavigationAction that causes a stopLoading (if a load was in progress)
and an immediate startLoading after that. I'm trying to postpone the stop/start
till we know whether the new loader will not immediately error out.

The alternative to this was adding a new callback (as here:
https://codereview.chromium.org/131363006/).

A third option is to rework the rest of chrome to support overlapping start/stop
notifications, but that's more work so I was hoping to avoid it if possible.

https://codereview.chromium.org/181493007/diff/20001/Source/core/loader/Frame...
Source/core/loader/FrameLoader.cpp:962: if (m_currentLoaderNeedsStop)
On 2014/03/10 16:12:44, Nate Chapin wrote:
> Why is this needed?

because otherwise we'll call
    m_progressTracker->progressCompleted();
before the delayed
     m_progressTracker->progressStarted();
is called.

https://codereview.chromium.org/181493007/diff/20001/Source/core/loader/Frame...
Source/core/loader/FrameLoader.cpp:1330: if (m_provisionalDocumentLoader)
On 2014/03/10 16:12:44, Nate Chapin wrote:
> Merge these 2 ifs? Or can stopLoading() null out the provisional document
> loader?

merge - sure. call stopLoading() - no, since that stops the documentLoader which
is what I'm trying to prevent.

[Nate Chapin, 2014-03-12 19:55:44]

https://codereview.chromium.org/181493007/diff/20001/Source/core/loader/Frame...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/181493007/diff/20001/Source/core/loader/Frame...
Source/core/loader/FrameLoader.cpp:637: m_progressTracker->progressStarted();
On 2014/03/11 23:24:08, mkosiba wrote:
> On 2014/03/10 16:12:44, Nate Chapin wrote:
> > It doesn't seem right to wait to send a progress tracker start notification
> > until a response is received.
> 
> this is the meat of the change. The problem is that when you call
> loadWithNavigationAction that causes a stopLoading (if a load was in progress)
> and an immediate startLoading after that. I'm trying to postpone the
stop/start
> till we know whether the new loader will not immediately error out.
> 
> The alternative to this was adding a new callback (as here:
> https://codereview.chromium.org/131363006/).
> 
> A third option is to rework the rest of chrome to support overlapping
start/stop
> notifications, but that's more work so I was hoping to avoid it if possible.

I talked with darin, and our thoughts were:

1. Let's start by seeing how much of a perf impact it would be to just let both
the XHR and the main resource run in parallel. Maybe run some perf measurements
or see what the network stack folks think?

2. If that's going to hurt perf too badly, we could consider using the load
deferral concept in blink. Blink has a way to defer all loading in a page, but
we could use most of that logic to defer just FrameLoader::m_documentLoader
while letting FrameLoader::m_provisionalDocumentLoader proceed normally.

3. If that still causes perf problems, we could look at making load deferral
more aggressively throttle in-progress loads.

Does that seem reasonable?

[mkosiba (inactive), 2014-03-14 19:13:52]

Thanks Nate!

On 2014/03/12 19:55:44, Nate Chapin wrote:
> I talked with darin, and our thoughts were:
> 
> 1. Let's start by seeing how much of a perf impact it would be to just let
both
> the XHR and the main resource run in parallel. Maybe run some perf
measurements
> or see what the network stack folks think?

just so that we're on the same page - you mean running both the current and
provisional
document loaders in parallel?

> 2. If that's going to hurt perf too badly, we could consider using the load
> deferral concept in blink. Blink has a way to defer all loading in a page, but
> we could use most of that logic to defer just FrameLoader::m_documentLoader
> while letting FrameLoader::m_provisionalDocumentLoader proceed normally.
> 
> 3. If that still causes perf problems, we could look at making load deferral
> more aggressively throttle in-progress loads.
> 
> Does that seem reasonable?

That sounds reasonable and I appreciate you talking with Darin on my behalf.

From my understanding of the code option 1) seems like it would require
more complex changes to the code than 2) though. The FrameLoader is built with
the
assumption that only one loader runs at a time, right?

The thing that concerns me the most is changing the
didStopLoading/didStartLoading
callback semantics. The way I think this works is:

if no load is in progress:
FrameLoader::load causes a didStartLoading
FrameLoader::checkLoadComplete/receivedMainResourceError causes an
didStopLoading

if a load is in progress:
FrameLoader::load causes an didStopLoading and an immediate didStartLoading
FrameLoader::checkLoadComplete/receivedMainResourceError causes an
didStopLoading

Having the main and provisional loaders run in parallel means we could have
overlapping
loads for the same WebContents:

main loader:            [start =================> end]
provisional loader:                     [start =================> end]

right?

[Nate Chapin, 2014-03-18 18:08:25]

On 2014/03/14 19:13:52, mkosiba wrote:
> Thanks Nate!
> 
> On 2014/03/12 19:55:44, Nate Chapin wrote:
> > I talked with darin, and our thoughts were:
> > 
> > 1. Let's start by seeing how much of a perf impact it would be to just let
> both
> > the XHR and the main resource run in parallel. Maybe run some perf
> measurements
> > or see what the network stack folks think?
> 
> just so that we're on the same page - you mean running both the current and
> provisional
> document loaders in parallel?

Yeah.

> 
> > 2. If that's going to hurt perf too badly, we could consider using the load
> > deferral concept in blink. Blink has a way to defer all loading in a page,
but
> > we could use most of that logic to defer just FrameLoader::m_documentLoader
> > while letting FrameLoader::m_provisionalDocumentLoader proceed normally.
> > 
> > 3. If that still causes perf problems, we could look at making load deferral
> > more aggressively throttle in-progress loads.
> > 
> > Does that seem reasonable?
> 
> That sounds reasonable and I appreciate you talking with Darin on my behalf.
> 
> From my understanding of the code option 1) seems like it would require
> more complex changes to the code than 2) though. The FrameLoader is built with
> the
> assumption that only one loader runs at a time, right?
> 
> The thing that concerns me the most is changing the
> didStopLoading/didStartLoading
> callback semantics. The way I think this works is:
> 
> if no load is in progress:
> FrameLoader::load causes a didStartLoading
> FrameLoader::checkLoadComplete/receivedMainResourceError causes an
> didStopLoading
> 
> if a load is in progress:
> FrameLoader::load causes an didStopLoading and an immediate didStartLoading
> FrameLoader::checkLoadComplete/receivedMainResourceError causes an
> didStopLoading
> 
> Having the main and provisional loaders run in parallel means we could have
> overlapping
> loads for the same WebContents:
> 
> main loader:            [start =================> end]
> provisional loader:                     [start =================> end]
> 
> right?

I think the internals of blink are resilient to having multiple DocumentLoaders
in progress, but you may be right that the rest of chromium is not ready for it.

So yes, perhaps #2 is a better place to start.

[mkosiba (inactive), 2014-03-19 21:50:43]

On 2014/03/18 18:08:25, Nate Chapin wrote:
> I think the internals of blink are resilient to having multiple
DocumentLoaders
> in progress, but you may be right that the rest of chromium is not ready for
it.

Sorry for not being clear on this. Yes, it's the rest of Chromium that I was
concerned
about.

> So yes, perhaps #2 is a better place to start.

Well, if we assume that the rest of Chromium is not ready for this then I don't
think
#2 helps - the problem is that we will start the new load without immediately
stopping
the old one. I don't think pausing the new load helps in this scenario.

That's why my approach till now was the inverse to what Darin
proposed -  not "officially" starting the new load till we knew whether the
associated network request had progressed beyond a certain point.

[Nate Chapin, 2014-03-19 22:05:14]

On 2014/03/19 21:50:43, mkosiba wrote:
> On 2014/03/18 18:08:25, Nate Chapin wrote:
> > I think the internals of blink are resilient to having multiple
> DocumentLoaders
> > in progress, but you may be right that the rest of chromium is not ready for
> it.
> 
> Sorry for not being clear on this. Yes, it's the rest of Chromium that I was
> concerned
> about.
> 
> > So yes, perhaps #2 is a better place to start.
> 
> Well, if we assume that the rest of Chromium is not ready for this then I
don't
> think
> #2 helps - the problem is that we will start the new load without immediately
> stopping
> the old one. I don't think pausing the new load helps in this scenario.
> 
> That's why my approach till now was the inverse to what Darin
> proposed -  not "officially" starting the new load till we knew whether the
> associated network request had progressed beyond a certain point.

Actually, now that I look at it, I think making interleaved start/stop
notifcations safe should be relatively straightforward. Probably as simple as
making RenderFrameImpl::is_loading_ a counter instead of a boolean, then we only
actually act on the start/stop notification if the calls are balanced.

If that is in fact all it takes, that's likely to be less of a pain than dealing
with the weburlloader callback or delaying start substantially.

[mkosiba (inactive), 2014-03-24 18:44:36]

On 2014/03/19 22:05:14, Nate Chapin wrote:
> Actually, now that I look at it, I think making interleaved start/stop
> notifcations safe should be relatively straightforward. Probably as simple as
> making RenderFrameImpl::is_loading_ a counter instead of a boolean, then we
only
> actually act on the start/stop notification if the calls are balanced.
> 
> If that is in fact all it takes, that's likely to be less of a pain than
dealing
> with the weburlloader callback or delaying start substantially.

Ok, so I wanted to create a counter-example for this but turns out the
didStart/stop
APIs aren't tied to navigations as tightly as I expected. I was concerned that
code written in the WebContentsObserver::didStart/StopLoad and
RenderViewObserver
would fail in subtle ways if we don't issue a stop+start when loading a new URL
but turns out that we already don't issue a stop+start for redirects so that
knocks out the major concern I had about this.

I'll give the above a shot, although I think keeping the counter in FrameLoader
would be a bit cleaner, no?

[Nate Chapin, 2014-03-24 21:59:56]

On 2014/03/24 18:44:36, mkosiba wrote:
> On 2014/03/19 22:05:14, Nate Chapin wrote:
> > Actually, now that I look at it, I think making interleaved start/stop
> > notifcations safe should be relatively straightforward. Probably as simple
as
> > making RenderFrameImpl::is_loading_ a counter instead of a boolean, then we
> only
> > actually act on the start/stop notification if the calls are balanced.
> > 
> > If that is in fact all it takes, that's likely to be less of a pain than
> dealing
> > with the weburlloader callback or delaying start substantially.
> 
> Ok, so I wanted to create a counter-example for this but turns out the
> didStart/stop
> APIs aren't tied to navigations as tightly as I expected. I was concerned that
> code written in the WebContentsObserver::didStart/StopLoad and
> RenderViewObserver
> would fail in subtle ways if we don't issue a stop+start when loading a new
URL
> but turns out that we already don't issue a stop+start for redirects so that
> knocks out the major concern I had about this.
> 
> I'll give the above a shot, although I think keeping the counter in
FrameLoader
> would be a bit cleaner, no?

That could work. The actually start/stop notifications are sent from two
different places (FrameLoader and ProgressTracker), depending on whether it's a
navigation within the existing document or to a different document. As long as
the FrameLoader implementation is resilient to that, it's fine.

[mkosiba (inactive), 2014-05-20 17:39:36]

OK, it looks like the ProgressTracker handles multiple calls to
progressStarted() correctly now so the above CL should be sufficient, PTAL.

[Nate Chapin, 2014-05-20 17:47:08]

I like the way this is shaping up. A couple of nits.

https://codereview.chromium.org/181493007/diff/60001/Source/core/loader/Frame...
File Source/core/loader/FrameLoader.cpp (left):

https://codereview.chromium.org/181493007/diff/60001/Source/core/loader/Frame...
Source/core/loader/FrameLoader.cpp:1296: if (!m_frame->page() ||
!m_policyDocumentLoader)
I don't think that removing this early return is safe.
DocumentLoader::stopLoading() doesn't do as much work as
FrameLoader::stopAllLoaders(), but I think if this is an iframe it can still
call into FrameLoader::checkCompleted(), which can call the parent's
checkCompleted(), which can fire the parent's load event and do work to trigger
the cases in this if().

https://codereview.chromium.org/181493007/diff/60001/Source/core/loader/Frame...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/181493007/diff/60001/Source/core/loader/Frame...
Source/core/loader/FrameLoader.cpp:1311: 
Any reason for the extra newlines here and below?

[mkosiba (inactive), 2014-05-20 19:06:09]

thanks!

https://codereview.chromium.org/181493007/diff/60001/Source/core/loader/Frame...
File Source/core/loader/FrameLoader.cpp (left):

https://codereview.chromium.org/181493007/diff/60001/Source/core/loader/Frame...
Source/core/loader/FrameLoader.cpp:1296: if (!m_frame->page() ||
!m_policyDocumentLoader)
On 2014/05/20 17:47:09, Nate Chapin wrote:
> I don't think that removing this early return is safe.
> DocumentLoader::stopLoading() doesn't do as much work as
> FrameLoader::stopAllLoaders(), but I think if this is an iframe it can still
> call into FrameLoader::checkCompleted(), which can call the parent's
> checkCompleted(), which can fire the parent's load event and do work to
trigger
> the cases in this if().

I will leave the check in. If I wanted to hit this scenario manually what would
be a good test case? An iframe that navigates the parent?

Also, when you fiddle with it, how do you test the code? I ended up running
browser_tests and content_browsertests but those are incredibly slooow.

https://codereview.chromium.org/181493007/diff/60001/Source/core/loader/Frame...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/181493007/diff/60001/Source/core/loader/Frame...
Source/core/loader/FrameLoader.cpp:1311: 
On 2014/05/20 17:47:09, Nate Chapin wrote:
> Any reason for the extra newlines here and below?

Done.

[mkosiba (inactive), 2014-05-20 19:06:10]

thanks!

https://codereview.chromium.org/181493007/diff/60001/Source/core/loader/Frame...
File Source/core/loader/FrameLoader.cpp (left):

https://codereview.chromium.org/181493007/diff/60001/Source/core/loader/Frame...
Source/core/loader/FrameLoader.cpp:1296: if (!m_frame->page() ||
!m_policyDocumentLoader)
On 2014/05/20 17:47:09, Nate Chapin wrote:
> I don't think that removing this early return is safe.
> DocumentLoader::stopLoading() doesn't do as much work as
> FrameLoader::stopAllLoaders(), but I think if this is an iframe it can still
> call into FrameLoader::checkCompleted(), which can call the parent's
> checkCompleted(), which can fire the parent's load event and do work to
trigger
> the cases in this if().

I will leave the check in. If I wanted to hit this scenario manually what would
be a good test case? An iframe that navigates the parent?

Also, when you fiddle with it, how do you test the code? I ended up running
browser_tests and content_browsertests but those are incredibly slooow.

https://codereview.chromium.org/181493007/diff/60001/Source/core/loader/Frame...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/181493007/diff/60001/Source/core/loader/Frame...
Source/core/loader/FrameLoader.cpp:1311: 
On 2014/05/20 17:47:09, Nate Chapin wrote:
> Any reason for the extra newlines here and below?

Done.

[Nate Chapin, 2014-05-20 19:16:05]

https://codereview.chromium.org/181493007/diff/60001/Source/core/loader/Frame...
File Source/core/loader/FrameLoader.cpp (left):

https://codereview.chromium.org/181493007/diff/60001/Source/core/loader/Frame...
Source/core/loader/FrameLoader.cpp:1296: if (!m_frame->page() ||
!m_policyDocumentLoader)
On 2014/05/20 19:06:10, mkosiba wrote:
> On 2014/05/20 17:47:09, Nate Chapin wrote:
> > I don't think that removing this early return is safe.
> > DocumentLoader::stopLoading() doesn't do as much work as
> > FrameLoader::stopAllLoaders(), but I think if this is an iframe it can still
> > call into FrameLoader::checkCompleted(), which can call the parent's
> > checkCompleted(), which can fire the parent's load event and do work to
> trigger
> > the cases in this if().
> 
> I will leave the check in. If I wanted to hit this scenario manually what
would
> be a good test case? An iframe that navigates the parent?
> 
> Also, when you fiddle with it, how do you test the code? I ended up running
> browser_tests and content_browsertests but those are incredibly slooow.

In terms of repro, I have never actually seen a reliable repro for either of
those cases. The m_frame->page() check has existed from time immemorial, and the
m_policyDocumentLoader check was in response to a top crasher I couldn't repro,
but fixed based on crash stacks. :(

In terms of testing, I usually run layout tests and webkit_unit_tests locally,
then send try jobs for everything else, and approximate "good enough to request
review" when the trybots have passed at least one bot.

[mkosiba (inactive), 2014-05-21 19:02:32]

so it turns out the change affects the webkit test runner slightly since it
captures the render tree from didFinishLoad. The output of tests that are doing
navigations from onload or drag files onto themselves are somewhat affected.

Patch Set 5 contains changes to tests that would make them pass without fiddling
with the test runner.

In https://codereview.chromium.org/297513010 (which requires Patch Set 6) I
tried fiddling with the test runner a bit and using didStartProvisionalLoad to
end the test seems to work fine provided I take out an ASSERT from the
FrameLoader (which seems regrettable).

Let me know which of the above would you prefer.

One thing I still can't figure out is why
fast/loader/unload-hyperlink-targeted.html times out in either of these
approaches. It runs fine if I run it manually in content_shell though...

[mkosiba (inactive), 2014-05-21 19:02:32]

so it turns out the change affects the webkit test runner slightly since it
captures the render tree from didFinishLoad. The output of tests that are doing
navigations from onload or drag files onto themselves are somewhat affected.

Patch Set 5 contains changes to tests that would make them pass without fiddling
with the test runner.

In https://codereview.chromium.org/297513010 (which requires Patch Set 6) I
tried fiddling with the test runner a bit and using didStartProvisionalLoad to
end the test seems to work fine provided I take out an ASSERT from the
FrameLoader (which seems regrettable).

Let me know which of the above would you prefer.

One thing I still can't figure out is why
fast/loader/unload-hyperlink-targeted.html times out in either of these
approaches. It runs fine if I run it manually in content_shell though...

[Nate Chapin, 2014-05-21 19:12:49]

On 2014/05/21 19:02:32, mkosiba wrote:
> so it turns out the change affects the webkit test runner slightly since it
> captures the render tree from didFinishLoad. The output of tests that are
doing
> navigations from onload or drag files onto themselves are somewhat affected.
> 
> Patch Set 5 contains changes to tests that would make them pass without
fiddling
> with the test runner.
> 
> In https://codereview.chromium.org/297513010 (which requires Patch Set 6) I
> tried fiddling with the test runner a bit and using didStartProvisionalLoad to
> end the test seems to work fine provided I take out an ASSERT from the
> FrameLoader (which seems regrettable).
> 
> Let me know which of the above would you prefer.
> 
> One thing I still can't figure out is why
> fast/loader/unload-hyperlink-targeted.html times out in either of these
> approaches. It runs fine if I run it manually in content_shell though...

I'd lean toward the Patch Set 6 approach, so long as the test runner owners are
ok with it.

If we have to go with the Patch Set 5 approach, we should try to figure out
something better than the 100ms setTimeout() calls.

[mkosiba (inactive), 2014-05-27 16:57:10]

On 2014/05/21 19:12:49, Nate Chapin wrote:
> On 2014/05/21 19:02:32, mkosiba wrote:
> > One thing I still can't figure out is why
> > fast/loader/unload-hyperlink-targeted.html times out in either of these
> > approaches. It runs fine if I run it manually in content_shell though...

Seems like this is a matter of a missing callback in the test runner. I'll
upload
a CL to add that in a bit.

> I'd lean toward the Patch Set 6 approach, so long as the test runner owners
are
> ok with it.

Turns out it's not that easy.. I tried a couple of approaches but it's always
a trade-off - I can fix some of the tests but some others break (initially I
didn't
notice since I was running a small set of them).

It seems that it's not possible (or really hard) to figure out
the exact moment when the test runner would have dumped text without this change
based on the callbacks with this change. All of my attempts to reproduce
"some" of the old behavior ended up not making things better (or outright
worse).

> If we have to go with the Patch Set 5 approach, we should try to figure out
> something better than the 100ms setTimeout() calls.

I changed the tests to prevent unload. I think it makes the most sense - the
tests should have been doing this from the beginning, it's just that the test
runner happens to DRT at one particular moment that allowed them to pass. WDYT?

[mkosiba (inactive), 2014-05-27 16:57:10]

On 2014/05/21 19:12:49, Nate Chapin wrote:
> On 2014/05/21 19:02:32, mkosiba wrote:
> > One thing I still can't figure out is why
> > fast/loader/unload-hyperlink-targeted.html times out in either of these
> > approaches. It runs fine if I run it manually in content_shell though...

Seems like this is a matter of a missing callback in the test runner. I'll
upload
a CL to add that in a bit.

> I'd lean toward the Patch Set 6 approach, so long as the test runner owners
are
> ok with it.

Turns out it's not that easy.. I tried a couple of approaches but it's always
a trade-off - I can fix some of the tests but some others break (initially I
didn't
notice since I was running a small set of them).

It seems that it's not possible (or really hard) to figure out
the exact moment when the test runner would have dumped text without this change
based on the callbacks with this change. All of my attempts to reproduce
"some" of the old behavior ended up not making things better (or outright
worse).

> If we have to go with the Patch Set 5 approach, we should try to figure out
> something better than the 100ms setTimeout() calls.

I changed the tests to prevent unload. I think it makes the most sense - the
tests should have been doing this from the beginning, it's just that the test
runner happens to DRT at one particular moment that allowed them to pass. WDYT?

[Nate Chapin, 2014-05-27 17:00:45]

On 2014/05/27 16:57:10, mkosiba wrote:
> On 2014/05/21 19:12:49, Nate Chapin wrote:
> > On 2014/05/21 19:02:32, mkosiba wrote:
> > > One thing I still can't figure out is why
> > > fast/loader/unload-hyperlink-targeted.html times out in either of these
> > > approaches. It runs fine if I run it manually in content_shell though...
> 
> Seems like this is a matter of a missing callback in the test runner. I'll
> upload
> a CL to add that in a bit.
> 
> > I'd lean toward the Patch Set 6 approach, so long as the test runner owners
> are
> > ok with it.
> 
> Turns out it's not that easy.. I tried a couple of approaches but it's always
> a trade-off - I can fix some of the tests but some others break (initially I
> didn't
> notice since I was running a small set of them).
> 
> It seems that it's not possible (or really hard) to figure out
> the exact moment when the test runner would have dumped text without this
change
> based on the callbacks with this change. All of my attempts to reproduce
> "some" of the old behavior ended up not making things better (or outright
> worse).
> 
> > If we have to go with the Patch Set 5 approach, we should try to figure out
> > something better than the 100ms setTimeout() calls.
> 
> I changed the tests to prevent unload. I think it makes the most sense - the
> tests should have been doing this from the beginning, it's just that the test
> runner happens to DRT at one particular moment that allowed them to pass.
WDYT?

Ok. LGTM

[mkosiba (inactive), 2014-05-28 17:50:41]

On 2014/05/27 16:57:10, mkosiba wrote:
> On 2014/05/21 19:12:49, Nate Chapin wrote:
> > On 2014/05/21 19:02:32, mkosiba wrote:
> > > One thing I still can't figure out is why
> > > fast/loader/unload-hyperlink-targeted.html times out in either of these
> > > approaches. It runs fine if I run it manually in content_shell though...
> 
> Seems like this is a matter of a missing callback in the test runner. I'll
> upload
> a CL to add that in a bit.

ok, so the CL in question makes the test runner also proceed on detachFrame
(https://codereview.chromium.org/293363016) but I'm wondering if it's not
masking some deeper problem.

It seems that if a child frame's FrameLoader is in the committed state[1] then
detaching it doesn't result in a didFinishLoad() callback for that frame (it
does
result in a checkLoadComplete for the parent). Is that expected?

[1] that is the detach happens between didCommitProvisionalLoad and
didFinishLoad
for that frame.

[Nate Chapin, 2014-05-28 18:02:51]

On 2014/05/28 17:50:41, mkosiba wrote:
> On 2014/05/27 16:57:10, mkosiba wrote:
> > On 2014/05/21 19:12:49, Nate Chapin wrote:
> > > On 2014/05/21 19:02:32, mkosiba wrote:
> > > > One thing I still can't figure out is why
> > > > fast/loader/unload-hyperlink-targeted.html times out in either of these
> > > > approaches. It runs fine if I run it manually in content_shell though...
> > 
> > Seems like this is a matter of a missing callback in the test runner. I'll
> > upload
> > a CL to add that in a bit.
> 
> ok, so the CL in question makes the test runner also proceed on detachFrame
> (https://codereview.chromium.org/293363016) but I'm wondering if it's not
> masking some deeper problem.
> 
> It seems that if a child frame's FrameLoader is in the committed state[1] then
> detaching it doesn't result in a didFinishLoad() callback for that frame (it
> does
> result in a checkLoadComplete for the parent). Is that expected?
> 
> [1] that is the detach happens between didCommitProvisionalLoad and
> didFinishLoad
> for that frame.

I would have thought that the detached frame would send a didFailLoad(), but its
parent will report a didFinishLoad(). I take it the didFailLoad isn't happening?

[mkosiba (inactive), 2014-05-28 22:51:31]

On 2014/05/28 18:02:51, Nate Chapin wrote: 
> I would have thought that the detached frame would send a didFailLoad(), but
its
> parent will report a didFinishLoad(). I take it the didFailLoad isn't
happening?

so after staring at this for a while it seems like the problem was in the
checkLoadCompleteForThisFrame call: the parent ended up being in the provisional
state when the child frame was calling checkCompleted(). This would cause
the parent's checkLoadCompleteForThisFrame to not call the child frame's
checkLoadCompleteForThisFrame and so it couldn't progress to the completed
state.

The fix I came up with was to switch the order around and call
checkLoadCompleteForThisFrame for all the child frames regardless of the
state the parent is in. It doesn't seem to cause any other test failures.

[Nate Chapin, 2014-05-28 23:00:37]

On 2014/05/28 22:51:31, mkosiba wrote:
> On 2014/05/28 18:02:51, Nate Chapin wrote: 
> > I would have thought that the detached frame would send a didFailLoad(), but
> its
> > parent will report a didFinishLoad(). I take it the didFailLoad isn't
> happening?
> 
> so after staring at this for a while it seems like the problem was in the
> checkLoadCompleteForThisFrame call: the parent ended up being in the
provisional
> state when the child frame was calling checkCompleted(). This would cause
> the parent's checkLoadCompleteForThisFrame to not call the child frame's
> checkLoadCompleteForThisFrame and so it couldn't progress to the completed
> state.
> 
> The fix I came up with was to switch the order around and call
> checkLoadCompleteForThisFrame for all the child frames regardless of the
> state the parent is in. It doesn't seem to cause any other test failures.

I think that's the way things used to be before I refactored some of this code.
If the tests are happy, LGTM.

[mkosiba (inactive), 2014-06-02 09:41:39]

Thanks for the review Nate! Let's see what happens when this lands...

https://codereview.chromium.org/181493007/diff/160001/LayoutTests/editing/pas...
File LayoutTests/editing/pasteboard/drag-files-to-editable-element-expected.txt
(right):

https://codereview.chromium.org/181493007/diff/160001/LayoutTests/editing/pas...
LayoutTests/editing/pasteboard/drag-files-to-editable-element-expected.txt:6:
FAIL result should be
<div>LayoutTests/editing/pasteboard/foo</div><div>LayoutTests/editing/pasteboard/bar</div><div>LayoutTests/editing/pasteboard/baz</div>.
Was .
FYI: this test didn't pass before my change either, it's just that the expected
result didn't contain the error message. I filed crbug.com/378716 to track this.

[mkosiba (inactive), 2014-06-02 09:41:44]

The CQ bit was checked by mkosiba@chromium.org

[commit-bot: I haz the power, 2014-06-02 09:42:05]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mkosiba@chromium.org/181493007/160001

[commit-bot: I haz the power, 2014-06-02 10:25:19]

Change committed as 175269