1433044 - Crash in `anonymous namespace'::AssertLoadingPrincipalAndClientInfoMatch

Status: VERIFIED FIXED

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

Description

[Calixte Denizet (:calixte)]

This bug was filed from the Socorro interface and is
report bp-0decce3d-7578-4dda-966a-01e010180125.
=============================================================

Top 10 frames of crashing thread:

0 xul.dll `anonymous namespace'::AssertLoadingPrincipalAndClientInfoMatch netwerk/base/nsNetUtil.cpp:260
1 xul.dll NS_NewChannel netwerk/base/nsNetUtil.cpp:305
2 xul.dll mozilla::dom::FetchDriver::HttpFetch dom/fetch/FetchDriver.cpp:524
3 xul.dll mozilla::dom::FetchDriver::Fetch dom/fetch/FetchDriver.cpp:397
4 xul.dll mozilla::dom::MainThreadFetchRunnable::Run dom/fetch/Fetch.cpp:444
5 xul.dll mozilla::ThrottledEventQueue::Inner::ExecuteRunnable xpcom/threads/ThrottledEventQueue.cpp:193
6 xul.dll mozilla::ThrottledEventQueue::Inner::Executor::Run xpcom/threads/ThrottledEventQueue.cpp:79
7 xul.dll mozilla::ThrottledEventQueue::Inner::ExecuteRunnable xpcom/threads/ThrottledEventQueue.cpp:193
8 xul.dll mozilla::ThrottledEventQueue::Inner::Executor::Run xpcom/threads/ThrottledEventQueue.cpp:79
9 xul.dll mozilla::SchedulerGroup::Runnable::Run xpcom/threads/SchedulerGroup.cpp:395

=============================================================

There are 32 crashes (from are 16 installations) in nightly 60 with buildid 20180124220129. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 1231211.

[1] https://hg.mozilla.org/mozilla-central/rev?node=60a29396c52f

Comment 1

[Pascal Chevrel:pascalc (PTO until April 26)]

In case it helps, a user reported that crash on the Nightly blog and provided an url where it crashes for him
https://blog.nightly.mozilla.org/2018/01/18/these-weeks-in-firefox-issue-30/#comment-4790

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Pascal, does that user have any extensions installed?

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

I guess crash stats does not show any extensions...

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

These are all fetch() calls from worker threads.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Ok, I have steps to reproduce:

1. Open http://example.com
2. Run this in the web console:

  new Worker('data:text/javascript,fetch("http://example.com")');

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Amazingly this means we don't have *any* tests that call fetch() from a data URL worker...

Comment 7

[Marion Daly [:mdaly]]

(In reply to Ben Kelly [:bkelly] from comment #6)
> Amazingly this means we don't have *any* tests that call fetch() from a data
> URL worker...

that's really surprising, we should include some as part of this bug.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Update local-url-inherit-controller.https.html to test fetch() interception in local URL windows and workers. r=asuth

Instead of writing a new test I decided to expand the scope of coverage in local-url-inherit-controller.https.html.  It was already creating data URL workers.  This both provokes the crash and reveals another bug.  While we're inheriting the service worker for local URL we're not actually intercepting subresource requests from it.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Make workers keep the same null principal even if GetChannelResultPrincipal() returns different null principals. r=baku

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8945522 [details] [diff] [review]
P1 Make workers keep the same null principal even if GetChannelResultPrincipal() returns different null principals. r=baku

Andrea, it turns out that this happens with data URL nsIChannels:

  nsCOMPtr<nsIPrincipal> p1;
  ssm->GetChannelResultPrincipal(channel, getter_AddRefs(p1));

  nsCOMPtr<nsIPrincipal> p2;
  ssm->GetChannelResultPrincipal(channel, getter_AddRefs(p2));

  // Not equal!
  p1->Equals(p2);

This is because GetChannelResultPrincipal() mints a new null principal each time AFAICT.

The ClientSource is created before we actually AsyncOpen() the nsIChannel.  So we get the principal once then.  We then rely on the idea that the principal will not change since workers are same-origin.  We end up getting a different null principal, though.

To work around this lets just avoid overwriting the null principal for a worker.  Once it has a null principal it just keeps it.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Update local-url-inherit-controller.https.html to test fetch() interception in local URL windows and workers. r=asuth

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Ben Kelly [:bkelly] from comment #8)
> workers.  This both provokes the crash and reveals another bug.  While we're
> inheriting the service worker for local URL we're not actually intercepting
> subresource requests from it.

The "other bug" was a problem in the test.  We're intercepting like we should.

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Update local-url-inherit-controller.https.html to test fetch() interception in local URL windows and workers. r=asuth

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8945530 [details] [diff] [review]
P2 Update local-url-inherit-controller.https.html to test fetch() interception in local URL windows and workers. r=asuth

Andrew, this patch further expands local-url-inherit-controller.https.html to run fetch() in the local URL clients to see if its intercepted.  This gives us better test coverage that service workers are actually working and also happens to trigger this crash.

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3543c226bebfa8ce3410c842c753d803d284c99b

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=12bc5f94f74c0d7e7458bc422e2e258f9a822056

Comment 17

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8945530 [details] [diff] [review]
P2 Update local-url-inherit-controller.https.html to test fetch() interception in local URL windows and workers. r=asuth

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

::: testing/web-platform/tests/service-workers/service-worker/resources/local-url-inherit-controller-frame.html
@@ +13,1 @@
>  </` + `script>`;

Makes you almost miss <![CDATA[, right? ;)

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8945522 [details] [diff] [review]
P1 Make workers keep the same null principal even if GetChannelResultPrincipal() returns different null principals. r=baku

Redirecting this to Andrew since Andrea is probably done for the day and this is a high frequency crasher.

Please see comment 10.

Comment 19

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8945522 [details] [diff] [review]
P1 Make workers keep the same null principal even if GetChannelResultPrincipal() returns different null principals. r=baku

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

Restating: This is a workaround for the interaction of bug 1433181 that we are minting a new null principal for each call to GetChannelResultPrincipal().  Bug 1431771 may also generally come into play because that one is about null principals with the same UUID not being equals.  The specific assertion crash triggers because the principals are not Equals() because they are separate instances, and then their stringified origins are not equal either (because they're also different UUIDs).  Those bugs will make this logic go away.

This logic is consistent with :bkelly's prior changes to WorkerPrivate r+'d by baku in bug 1333573 where WorkerPrivate::FinalChannelPrincipalIsValid which added the following logic:

  // Verify that the channel is still a null principal.  We don't care
  // if these are the exact same null principal object, though.  From
  // the worker's perspective its the same effect.
  if (principal->GetIsNullPrincipal() && mPrincipal->GetIsNullPrincipal()) {
    return true;
  }

  // Otherwise we require exact equality.  Redirects can happen, but they
  // are not allowed to change our principal.
  if (principal->Equals(mPrincipal)) {
    return true;
  }

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

> Bug 1431771 may also generally come into play because that one is about null principals
> with the same UUID not being equals.

Just to clarify, that is actually unrelated because we are getting different UUIDs as well.  So the origin string is different.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0e6a1684ad6
P1 Make workers keep the same null principal even if GetChannelResultPrincipal() returns different null principals. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/c48ec0d13931
P2 Update local-url-inherit-controller.https.html to test fetch() interception in local URL windows and workers. r=asuth

Comment 22

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/c0e6a1684ad6
https://hg.mozilla.org/mozilla-central/rev/c48ec0d13931

Comment 23

[Calixte Denizet (:calixte)]

No more crashes since the patches landed.