Closed Bug 1547524 Opened 5 years ago Closed 5 years ago

MOZ_RELEASE_ASSERT(ClientMatchPrincipalInfo(mClientInfo.PrincipalInfo(), aServiceWorker.PrincipalInfo()) with alert() before navigating

Categories

(Core :: DOM: Service Workers, defect, P1)

Product:
Core
Component:
DOM: Service Workers
Version:
67 Branch
Type:
defect

Tracking

()

Status:
RESOLVED DUPLICATE of bug 1535699
Tracking Flags:
Tracking Status
firefox-esr60 --- wontfix
firefox66 --- wontfix
firefox67 + wontfix
firefox68 + wontfix

People

(Reporter: emilio, Assigned: asuth)

References

(Regression)

Details

(5 keywords)

Crash Data

Attachments

(1 file, 1 obsolete file)

HTML file from comment 0 in downloadable form. Must be saved as to disk and viewed via file protocol to reproduce the scenario!
288 bytes, text/html
Details

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>

ni? asuth since he reviewed the assertion, and bkelly is no longer around.

Flags: needinfo?(bugmail)

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.

tracking-firefox67: --- → ?
tracking-firefox68: --- → ?

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.

Assignee: nobody → bugmail
Status: NEW → ASSIGNED
Flags: needinfo?(nika)
Flags: needinfo?(bugmail)
Depends on: 1535699
Crash Signature: [@ mozilla::dom::ClientSource::SetController ]
Keywords: crash, crashreportid, regression
Priority: -- → P1
Severity: normal → critical
status-firefox66: --- → unaffected
OS: Unspecified → All
Hardware: Unspecified → All
Version: unspecified → 67 Branch
Component: DOM: Core & HTML → DOM: Service Workers
See Also: → 1549580

(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?

Keywords: testcase
Flags: needinfo?(bugmail)

(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.

Flags: needinfo?(bugmail)

(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

marking as a duplicate of bug 1535699

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox67: affected → wontfix
status-firefox68: affected → wontfix
Resolution: --- → DUPLICATE
Attachment #9064866 - Attachment description: Bug 1547524 - Do process switch _before_ processing cached redirect r=mayhemer → Bug 1551601 - Do process switch _before_ processing cached redirect r=mayhemer

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.

Attachment #9064866 - Attachment is obsolete: true
Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: