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)
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)
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
bhsu
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bhsu
:
review+
|
Details | Diff | Splinter Review |
[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.
Updated•7 years ago
|
status-firefox56:
--- → unaffected
Assignee | ||
Comment 1•7 years ago
|
||
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.
Assignee | ||
Comment 2•7 years ago
|
||
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.
Assignee | ||
Comment 3•7 years ago
|
||
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.
Assignee | ||
Comment 4•7 years ago
|
||
In this case the assertion is getting triggered from an importScripts() which is allowed to redirect.
Assignee | ||
Comment 5•7 years ago
|
||
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 | ||
Comment 6•7 years ago
|
||
I'll add a WPT test as well.
Assignee | ||
Comment 7•7 years ago
|
||
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)
Assignee | ||
Comment 8•7 years ago
|
||
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)
Assignee | ||
Updated•7 years ago
|
Attachment #8907144 -
Flags: review?(amarchesini) → review?(bhsu)
Comment 9•7 years ago
|
||
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+
Updated•7 years ago
|
Attachment #8907144 -
Flags: review?(bhsu) → review+
Comment 10•7 years ago
|
||
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 :)
Assignee | ||
Comment 11•7 years ago
|
||
(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!
Comment 12•7 years ago
|
||
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
Updated•7 years ago
|
Comment 13•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4a4f563364a6
https://hg.mozilla.org/mozilla-central/rev/6c1bf4fea13b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Updated•7 years ago
|
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•