MOZ_RELEASE_ASSERT(ClientMatchPrincipalInfo(mClientInfo.PrincipalInfo(), aServiceWorker.PrincipalInfo()) with alert() before navigating
Categories
(Core :: DOM: Service Workers, defect, P1)
Tracking
()
People
(Reporter: emilio, Assigned: asuth)
References
(Regression)
Details
(5 keywords)
Crash Data
Attachments
(1 file, 1 obsolete file)
I was poking at fixing bug 1218456, and found this reproducible crash which looks like an issue and is reproducible with a clean profile.
STR:
- Create a new profile.
- Open the following test-case (I was using a
file://
uri if it matters):
<!doctype html>
<body>
<script>
let link = document.createElement("a");
link.href = "https://youtube.com";
link.addEventListener("DOMActivate", function() {
alert("DOMActivate");
});
link.innerText = "Browse";
document.body.appendChild(link);
// link.click();
</script>
- Click on
Browse
, dismiss thealert
. - If the profile is clean you'll navigate as expected.
- Wait for youtube to load completely.
- Browse back.
- Click on
Browse
, dismiss thealert
. - Crash: bp-0950f985-9b84-4f1e-bd6d-55bea0190427, bp-1745b0ef-f6f8-4948-be93-f67e30190427
Reporter | ||
Comment 1•5 years ago
|
||
ni? asuth since he reviewed the assertion, and bkelly is no longer around.
Reporter | ||
Comment 2•5 years ago
|
||
Mozregression tells me:
So bug 1528360.
Reporter | ||
Comment 3•5 years ago
|
||
I can also confirm that I can no longer reproduce it when I toggle that pref back to false.
[Tracking Requested - why for this release]: Regression in security release assertion introduced in 67. Note that I don't know much about the code involved, so may not be such a huge issue, Nika can probably assess how severe is this.
Updated•5 years ago
|
Assignee | ||
Comment 4•5 years ago
|
||
The file URI does matter as that triggers the process swap mechanism because http pages can't be opened in file processes.
I think this is fundamentally the same bug as bug 1530168 and where your file example will be very useful! Thank you! (It's much more involved to reproduce the webextension scenario realistically without cheating with the process selector and thereby perhaps making the test not useful.)
I will probably dupe this over, but taking for now.
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Comment 5•5 years ago
|
||
(In reply to Andrew Sutherland [:asuth] (he/him) from comment #4)
The file URI does matter as that triggers the process swap mechanism because http pages can't be opened in file processes.
I think this is fundamentally the same bug as bug 1530168 and where your file example will be very useful! Thank you! (It's much more involved to reproduce the webextension scenario realistically without cheating with the process selector and thereby perhaps making the test not useful.)
I will probably dupe this over, but taking for now.
Andrew, so this is a duplicate of bug 1530168, which was marked as a duplicate of bug 1535699 which had a parch uplifted to beta which didn't fix the crashes.
This bug has a testcase and I can reproduce the crash in 67.0b17 (bp-217406a2-d3db-415d-9b44-9a5180190507) but not in nightly. Was the testcase here used to check if the patch in bug 1535699 could work?
Updated•5 years ago
|
Assignee | ||
Comment 6•5 years ago
•
|
||
(updated Pascal via IRC so clearing that needinfo)
Perry and I just finished up some good pair rr debugging of his reproduction, although there's more to do still. (Unfortunately pernos.co would make this much easier but we haven't pursued it yet because we'd expect the test to fail in automation for reasons of not being able to contact the real youtube.com servers. Now that we have a better understanding of exactly how the test is going wrong we should potentially be able to adapt our existing test to this scenario against beta and get a pernos.co trace.)
For my benefit tomorrow, the current scenario as I understand it is that:
- The "https://youtube.com/" -> "https://www.youtube.com/" redirect is being initially processed in the remoteType="file" content process. We would expect this to trigger the content process swap before processing the redirect, but it does not.
- The "file" content process is getting to HttpChannelChild::SetupRedirect() and invoking HttpChannelChild::ForceIntercepted(true, false)[1] which marks the redirection for interception post-redirection.
- While that redirect is then being processed in the parent, a process swap is triggered.
- So we're ending up with an HttpChannelChild for "https://www.youtube.com" in the remoteType="content" content process, which is what we expect. But because the channel is marked for post-redirect interception, the redirect does not do the normal redirect thing where it consults the content process, and so ClientChannelHelper::AsyncOnChannelRedirect is definitely not being invoked. And that's the piece that would clear the client.
- It's also the case that child intercept 100% does not expect the redirect to trigger a process swap. That wildly violates its understanding of how things work.
- It's possible there are complications related to the fact that there are 2 spots in nsHttpChannel where StartCrossProcessRedirect can be invoked. (OnStartRequest, and ContinueProcessResponse1 which I believe is post-start-request.)
1: https://searchfox.org/mozilla-central/rev/99a2a5a955960b0e58ceade1db1f7652d9db4ba1/netwerk/protocol/http/HttpChannelChild.cpp#1665 but that's trunk code and we're debugging beta, so be aware.
Comment 7•5 years ago
|
||
(In reply to Pascal Chevrel:pascalc from comment #5)
This bug has a testcase and I can reproduce the crash in 67.0b17 (bp-217406a2-d3db-415d-9b44-9a5180190507) but not in nightly. Was the testcase here used to check if the patch in bug 1535699 could work?
FWIW I can also reproduce it in Nightly:
https://crash-stats.mozilla.org/report/index/f159afe5-e4ae-49b7-ac06-aec380190508
Comment 8•5 years ago
|
||
marking as a duplicate of bug 1535699
Assignee | ||
Comment 9•5 years ago
|
||
Comment 10•5 years ago
|
||
Updated•5 years ago
|
Comment 11•5 years ago
|
||
Comment on attachment 9064866 [details]
Bug 1551601 - Do process switch before processing cached redirect r=mayhemer
Revision D31129 was moved to bug 1551601. Setting attachment 9064866 [details] to obsolete.
Updated•3 years ago
|
Description
•