1598497 - Consider moving nsIHttpChannelInternal.canceled to nsIChannel

Status: RESOLVED FIXED

(Core :: Networking: HTTP, task, P2)

Description

[(no longer active)]

I noticed in the debugger that a QI in nsAsyncRedirectVerifyHelper::IsOldChannelCanceled fails on a DocumentChannelChild, whereas this channel definitely has a cancelled status and might be cancelled here, so this looked like a bug, though I'm unsure of its implications.

Comment 1

[Jean-Yves Avenard [:jya]]

(In reply to :ehsan akhgari from comment #0)

I noticed in the debugger that a QI in nsAsyncRedirectVerifyHelper::IsOldChannelCanceled fails on a DocumentChannelChild, whereas this channel definitely has a cancelled status and might be cancelled here, so this looked like a bug, though I'm unsure of its implications.

What do you mean by failing?

nsAsyncRedirectVerifyHelper::IsOldChannelCanceled will QI nsIHttpChannelInternal and if that fails will default to checking the status instead.

This fallback will work just fine with DocumentChannelChild, as calling DCC::Cancel(status) will set the status to an error code, and as such we will get to there:
https://searchfox.org/mozilla-central/rev/652014ca1183c56bc5f04daf01af180d4e50a91c/netwerk/base/nsAsyncRedirectVerifyHelper.cpp#288

I'm happy to move GetCanceled to nsIChannel, as I believe it makes for a more elegant solution than the one in place, but it's an expansive change.

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1598497 - P1. move canceled attribute to nsIChannel. r?mayhemer

There is no functional change with this commit. The default implementation for GetCanceled() is still to check if the status code is a failure.

However, it can be argued that as you had to call Cancel() on the nsIChannel, having to check the nsIHttpChannelInternal interface to determine if you had been canceled in the past was rather a non obvious path.

It makes more sense to check the nsIChannel interface to determine if it's been canceled already and this allows for finer granularity if needed in the future.

Comment 3

[(no longer active)]

(In reply to Jean-Yves Avenard [:jya] from comment #1)

(In reply to :ehsan akhgari from comment #0)

I noticed in the debugger that a QI in nsAsyncRedirectVerifyHelper::IsOldChannelCanceled fails on a DocumentChannelChild, whereas this channel definitely has a cancelled status and might be cancelled here, so this looked like a bug, though I'm unsure of its implications.

What do you mean by failing?

I mean at https://searchfox.org/mozilla-central/rev/652014ca1183c56bc5f04daf01af180d4e50a91c/netwerk/base/nsAsyncRedirectVerifyHelper.cpp#276-277 mOldChan is pointing to a DocumentChannelChild, and that line calls DocumentChannelChild::QueryInterface() asking for nsIHttpChannelInternal and gets a nullptr out.

nsAsyncRedirectVerifyHelper::IsOldChannelCanceled will QI nsIHttpChannelInternal and if that fails will default to checking the status instead.

This fallback will work just fine with DocumentChannelChild, as calling DCC::Cancel(status) will set the status to an error code, and as such we will get to there:
https://searchfox.org/mozilla-central/rev/652014ca1183c56bc5f04daf01af180d4e50a91c/netwerk/base/nsAsyncRedirectVerifyHelper.cpp#288

Perhaps I'm misunderstanding how this code works, but when DocumentChannelChild::mCanceled is set it seems that DocumentChannelChild::mStatus is not set immediately. (I think the parent will end up notifying the child later and that's when mStatus gets set.) So it wasn't obvious to me if the fallback path is indeed working as expected here.

I'm happy to move GetCanceled to nsIChannel, as I believe it makes for a more elegant solution than the one in place, but it's an expansive change.

FWIW my suggestion may be the wrong way to fix the bug, it was the best I could come up with. :-)

Comment 4

[(no longer active)]

BTW if my understanding here is correct, there is indeed a functional change in the patch you've posted here, FWIW.

Comment 5

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1598497 - P2. Have the canceled attribute only if explicitly canceled. r?mayhemer.

For all but nsBaseChannel implementation, GetCanceled will only return true if Cancel() was called.

With nsBaseChannel however, following a call to OnStopRequest and OnRedirectVerifyCallback, GetCanceled() would now return true. Is this intented.

With some methods, calling Cancel(NS_OK) will now make a call to GetCanceled() return true which is a change with earlier behaviour which only returned true if the error code was an actual error.

Depends on D55268

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/81beebdc2202
P1. move canceled attribute to nsIChannel. r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/3b31bbf74a5f
P2. Have the canceled attribute only if explicitly canceled. r=mayhemer.

Comment 7

[Noemi Erli[:noemi_erli]]

Backed out 2 changesets (Bug 1598497) for causing OSX build bustages CLOSED TREE

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=281646768&resultStatus=testfailed%2Cbusted%2Cexception&revision=3b31bbf74a5f706f17031b44ace32499a87c7ab6

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=281646722&repo=autoland&lineNumber=47818

Backout: https://hg.mozilla.org/integration/autoland/rev/c1bcdbe2393480301d11f8976f4589407897ae60

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/37aaa3438c41
P1. move canceled attribute to nsIChannel. r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/9f7e7ee68891
P2. Have the canceled attribute only if explicitly canceled. r=mayhemer.

Comment 9

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/37aaa3438c41
https://hg.mozilla.org/mozilla-central/rev/9f7e7ee68891