644185 - Redirect race may result in channel continuing to load after user hits "Stop" button.

Status: RESOLVED INVALID

(Core :: Networking: HTTP, defect)

Description

[Jason Duell]

Attachment: Simulate user hitting stop button in middle of redirect.

A user can hit "stop" at any point during a load, and presumably we always want to stop the channel load, even if we're in the middle of a redirect.

It appears that if the user hits Stop in the middle of our redirect logic, it's possible that they will simply cancel the old channel, but the new channel will get approved, and then keep loading.  I.e. we should logically always propagate any Cancel of the old channel to the new channel until the moment the new channel becomes the channel in use (at which point any Cancel from hitting Stop will go directly to it), but there's currently a window where that's not the case.

I've attached an xpcshell test that simulates this by having a redirect observer cancel the old channel (but still approve the redirect).  The test currently fails, as the new channel loads without being cancelled.

I thought at first this was probably a regression from our move to the async redirect API, but I just wound my repo back to the last changeset before the async API landed (d9504ca06476) and the error is there too.  So this isn't a regression.  But I do think it's a minor bug.   Unless I'm missing something?

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

Pressing stop actually cancels the load group. the new channel should be in the load group and therefore get cancelled. I suspect if you change your testcase to use a load group and cancel that, you'd see the correct behaviour - maybe try that?

Comment 2

[Honza Bambas (:mayhemer)]

Attachment: fix for the test

Update you test with this patch and it will work ;)  Self explanatory...

Comment 3

[Honza Bambas (:mayhemer)]

Closing as invalid.  The premiss was based on faulty test.  If the new channel is in a load group, then both (or at least one of the new or old) channels will get canceled.  There is atomically ensured that both channels are in a load group for at least a moment.  So, stopping the page load in an event loop right before AsyncOpening the new channel or right after it will always be propagated to either the new or the old channel.

Comment 4

[Honza Bambas (:mayhemer)]

(In reply to comment #3)
> are in a load
> group for at least a moment

meaning together.  I wanted to say, that there is no event loop while none of the channels (new or old) would not be in the load group.