Closed Bug 1398484 Opened 7 years ago Closed 7 years ago

Assertion failure: channel == loadInfo.mChannel, at dom/workers/ScriptLoader.cpp:683

Categories

(Core :: DOM: Core & HTML, defect)

57 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 - fixed

People

(Reporter: bc, Assigned: bkelly)

References

()

Details

(Keywords: assertion, regression, regressionwindow-wanted)

Attachments

(3 files)

Attached file Linux assertion log
[Tracking Requested - why for this release]: regression

1. https://react-hn.appspot.com/#/%3F_k=2omvf9

2. Assertion failure: channel == loadInfo.mChannel, at /builds/worker/workspace/build/src/dom/workers/ScriptLoader.cpp:683

Windows / Linux Nightly 57 but not Beta 56 -> regression.
I think this assertion might be bogus.  If there is an internal redirect then the channel will not be the same.  Also, I believe service worker scripts can redirect same-origin.  We probably don't test that, though.
Actually, service worker scripts are not supposed to allow redirect, but I don't see where that is prevented currently.

The "regression" here, though, is probably that we are doing an extra internal redirect that we don't expect.
We block redirects by setting the nsIChannel redirect limit to 0.  I believe internal redirects will decrement the limit count, but we don't block internal redirects for the count.  So I think we might need to support internal redirects in ScriptLoader.cpp.
In this case the assertion is getting triggered from an importScripts() which is allowed to redirect.
This patch updates the service worker ScriptLoader code to expect redirects for importScripts().  It mostly worked before except for an assertion.  It seems we do still use loadInfo->mChannel in a couple places, so I updated that to the new channel as well.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff9339b09af4f4032f908b45d7a1936db012da94
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Attachment #8907136 - Flags: review?(amarchesini)
I'll add a WPT test as well.
This patch adds a WPT test to exercise the importScripts() redirect path.  It seems we didn't have any test coverage for it before.  This test crashes on debug builds without the P1 patch applied.  It passes with the P1 patch applied.
Attachment #8907144 - Flags: review?(amarchesini)
Comment on attachment 8907136 [details] [diff] [review]
Only assert that the channel does not change for top level service worker scripts. r=baku

Actually, Ho-Pang, do you mind reviewing these patches since you've been looking at the script loading code lately?
Attachment #8907136 - Flags: review?(amarchesini) → review?(bhsu)
Attachment #8907144 - Flags: review?(amarchesini) → review?(bhsu)
Comment on attachment 8907136 [details] [diff] [review]
Only assert that the channel does not change for top level service worker scripts. r=baku

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

::: dom/workers/ScriptLoader.cpp
@@ +684,5 @@
> +    // Note that importScripts() can redirect.  In theory the main
> +    // script could also encounter an internal redirect, but currently
> +    // the assert does not allow that.
> +    MOZ_ASSERT_IF(mIsMainScript, channel == loadInfo.mChannel);
> +    loadInfo.mChannel = channel;

Nice catch!

It's kind of interesting that there was nothing bad perceivable happened caused by saving the headers from the wrong channel[0].

After discussing with schien, I think making LoaderListener[1] implement nsIChannelEventSink and doing the checks and the assignment in asyncOnChannelRedirect could be another viable solution, which is more explicit but could be a little overkilled. I am totally fine with current path, but if you think it's a good idea, maybe we can create a good-first-bug for it.

[0] http://searchfox.org/mozilla-central/source/dom/workers/ScriptLoader.cpp#718
[1] http://searchfox.org/mozilla-central/source/dom/workers/ScriptLoader.cpp#517
Attachment #8907136 - Flags: review?(bhsu) → review+
Attachment #8907144 - Flags: review?(bhsu) → review+
Comment on attachment 8907136 [details] [diff] [review]
Only assert that the channel does not change for top level service worker scripts. r=baku

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

I forgot to bring this up in last comment.
Maybe we can place "P1" preceding the comment message.

Thanks :)
(In reply to Ben Hsu [:HoPang] from comment #9)
> It's kind of interesting that there was nothing bad perceivable happened
> caused by saving the headers from the wrong channel[0].

Yea, I think I added that code so we got CSP stuff on main channels.  I don't think the headers are directly used for importScripts() yet.  We save them, though, in case we need them in the future.

> After discussing with schien, I think making LoaderListener[1] implement
> nsIChannelEventSink and doing the checks and the assignment in
> asyncOnChannelRedirect could be another viable solution, which is more
> explicit but could be a little overkilled. I am totally fine with current
> path, but if you think it's a good idea, maybe we can create a
> good-first-bug for it.

I think I'm going to stick with this current change for now.  I agree its a bit off this code does not listen for redirects since it wants to block some redirects and allow others.  For now, though, I don't have time to do any refactoring here.  Also, I think we might want to move the "redirect mode = error" logic into nsIChannel itself instead of requiring outside redirect listeners to enforce it.

Thanks for the review!
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a4f563364a6
Only assert that the channel does not change for top level service worker scripts. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c1bf4fea13b
P2 Add a WPT test that verifies importScripts() can be redirected. r=baku
Blocks: 1400372
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.