Closed Bug 1179399 Opened 5 years ago Closed 5 years ago

Assert/Crash running "fetch-event-network-error.https.html" wpt service worker test in Nightly

Categories

(Core :: DOM: Service Workers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox42 --- affected
firefox43 --- fixed

People

(Reporter: noemi, Assigned: jdm)

References

Details

Attachments

(4 files)

A Nightly crash occurs when executing "fetch-event-network-error.https.html" wpt test such as |./mach web-platform-tests _mozilla/service-workers/service-worker/fetch-event-network-error.https.html| with today's (7/1) master build

The assertion failure shown is as follows:
"Assertion failure: ShouldIntercept() (This can only be called on channels that can be intercepted), at /Users/noef/Documents/mozilla-central/netwerk/protocol/http/HttpBaseChannel.cpp:1427"

Please find attached the crash report corresponding to this
Assignee: nobody → ehsan
The issue here is that OverrideSecurityInfo/OverrideURI can be called after the content viewer of the docshell has been destroyed, so our attempt to get the document from the docshell here can always fail. <https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#14077>

Therefore we cannot reliably say whether a channel is intercepted in that function, so there is no point in having these ShouldIntercept() checks.
Note that we have seen this in the wild too, see bug 1173378.
Blocks: 1173378
Attachment #8628524 - Flags: review?(michal.novotny)
Attachment #8628520 - Flags: review?(josh) → review+
Keywords: leave-open
Comment on attachment 8628520 [details] [diff] [review]
Part 1: Relax the ShouldIntercept checks when overriding JAR channel info

https://hg.mozilla.org/integration/mozilla-inbound/rev/9ba6a55ec905
Attachment #8628520 - Flags: checkin+
Comment on attachment 8628524 [details] [diff] [review]
Part 2: Relax the ShouldIntercept checks when overriding HTTP channel info; r=michal

The check was required by Honza (bug #1133763 comment #23).
Attachment #8628524 - Flags: review?(michal.novotny) → review?(honzab.moz)
(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #1)
> The issue here is that OverrideSecurityInfo/OverrideURI can be called after
> the content viewer of the docshell has been destroyed, so our attempt to get
> the document from the docshell here can always fail.
> <https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.
> cpp#14077>
> 
> Therefore we cannot reliably say whether a channel is intercepted in that
> function, so there is no point in having these ShouldIntercept() checks.

That's pretty sad.. :(  Is there some other way so that we can at that point find out the channel is synthesized?  I'd rather keep at least some check..  But if too complicated then let's remove the check/assert for the sake of the goal.
comment 8 ^^^
Flags: needinfo?(ehsan)
(In reply to Honza Bambas (:mayhemer) from comment #8)
> (In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail,
> needinfo? me!) from comment #1)
> > The issue here is that OverrideSecurityInfo/OverrideURI can be called after
> > the content viewer of the docshell has been destroyed, so our attempt to get
> > the document from the docshell here can always fail.
> > <https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.
> > cpp#14077>
> > 
> > Therefore we cannot reliably say whether a channel is intercepted in that
> > function, so there is no point in having these ShouldIntercept() checks.
> 
> That's pretty sad.. :(  Is there some other way so that we can at that point
> find out the channel is synthesized?  I'd rather keep at least some check.. 
> But if too complicated then let's remove the check/assert for the sake of
> the goal.

I don't think there is a way to do that.  Josh, can you confirm, please?
Flags: needinfo?(ehsan) → needinfo?(josh)
We could add a separate boolean to HttpBaseChannel and set it from appropriate places in nsHttpChannel and HttpChannelChild, but otherwise I can't think of anything.
Flags: needinfo?(josh)
Honza, please let me know which approach you would prefer.  Thanks!
Flags: needinfo?(honzab.moz)
jdm, if you have time to add a flag then do so.  But if that turns out too complicated, let's just remove the assertion.
Flags: needinfo?(honzab.moz)
Comment on attachment 8628524 [details] [diff] [review]
Part 2: Relax the ShouldIntercept checks when overriding HTTP channel info; r=michal

Review of attachment 8628524 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab, if there is no better solution
Attachment #8628524 - Flags: review?(honzab.moz) → review+
(In reply to Honza Bambas (:mayhemer) from comment #13)
> jdm, if you have time to add a flag then do so.  But if that turns out too
> complicated, let's just remove the assertion.
Flags: needinfo?(josh)
Status: NEW → ASSIGNED
Josh: ping?
This should do the trick.
Attachment #8637529 - Flags: review?(honzab.moz)
Assignee: ehsan → josh
Assignee: josh → ehsan
Flags: needinfo?(josh)
All yours.  :-)
Assignee: ehsan → josh
Comment on attachment 8637529 [details] [diff] [review]
Add a flag to HttpBaseChannel indicating whether interception is occurring

Review of attachment 8637529 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

::: netwerk/protocol/http/HttpBaseChannel.h
@@ +378,5 @@
>  
>    // True if this channel should skip any interception checks
>    uint32_t                          mForceNoIntercept           : 1;
>  
> +  // True if this channel was intercepted could receive a synthesized response.

intercepted _and_ could ?
Attachment #8637529 - Flags: review?(honzab.moz) → review+
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/72db22eb14fe
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Hi,

I've just tested this in m-c (ba43a48d3c52 revision) and there is no a crash/assertion but the test still fails, please see below the details:

Summary

Harness status: OK

Found 1 tests
1 Fail
Details
*Result	
**Fail

*Test Name
**Rejecting the fetch event or using preventDefault() causes a network error	assert_equals: expected "PASS" but got "FAIL: prevent-default: expected network error but loaded"

*Message
**@https://web-platform.test:8443/_mozilla/service-workers/service-worker/fetch-event-network-error.https.html:37:11
promise callback*@https://web-platform.test:8443/_mozilla/service-workers/service-worker/fetch-event-network-error.https.html:24:1
Test.prototype.step@https://web-platform.test:8443/resources/testharness.js:1380:20
promise callback*promise_test@https://web-platform.test:8443/resources/testharness.js:526:31
@https://web-platform.test:8443/_mozilla/service-workers/service-worker/fetch-event-network-error.https.html:19:1


Josh, is that being tracked in other bug or should we open a new one?. Thanks!
Flags: needinfo?(josh)
That sounds like it's worth opening separately.
Flags: needinfo?(josh)
(In reply to Josh Matthews [:jdm] from comment #23)
> That sounds like it's worth opening separately.

ok, done. Please see bug 1198230. Thanks!
See Also: → 1198230
Duplicate of this bug: 1185513
You need to log in before you can comment on or make changes to this bug.