1495285 - Crash in mozilla::dom::ClientSource::WindowExecutionReady

Status: RESOLVED FIXED

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

Description

[Benoit Girard (:BenWa)]

This bug was filed from the Socorro interface and is
report bp-dfff99f7-e92d-4c0e-8aff-3e06f0180930.
=============================================================

Top 10 frames of crashing thread:

0 xul.dll mozilla::dom::ClientSource::WindowExecutionReady dom/clients/manager/ClientSource.cpp:286
1 xul.dll nsGlobalWindowOuter::SetNewDocument dom/base/nsGlobalWindowOuter.cpp:1950
2 xul.dll nsresult nsDocumentViewer::InitInternal layout/base/nsDocumentViewer.cpp:1032
3 xul.dll nsDocumentViewer::Init layout/base/nsDocumentViewer.cpp:771
4 xul.dll nsresult nsDocShell::Embed docshell/base/nsDocShell.cpp:6706
5 xul.dll nsDSURIContentListener::DoContent docshell/base/nsDSURIContentListener.cpp:196
6 xul.dll bool nsDocumentOpenInfo::TryContentListener uriloader/base/nsURILoader.cpp:759
7 xul.dll nsDocumentOpenInfo::OnStartRequest uriloader/base/nsURILoader.cpp:306
8 xul.dll void mozilla::net::HttpChannelChild::DoOnStartRequest netwerk/protocol/http/HttpChannelChild.cpp:771
9 xul.dll void mozilla::net::HttpChannelChild::OnStartRequest netwerk/protocol/http/HttpChannelChild.cpp:638

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

This is reproducable by loading this site and waiting 2 seconds:
https://forums.2k.com/showthread.php?86547-Space-Race-launch-location&highlight=spaceship

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Andrew, could you take a look at this?  MOZ_DIAGNOSTIC_ASSERT firing is really bad.

Comment 2

[Sam Sneddon [:gsnedders]]

see also bug 1487227, recently resolved for the same stack (as a duplicate of the fixed bug 1493211)

Comment 3

[Benoit Girard (:BenWa)]

Just hit it again and can reproduce it on this URL:
https://knowyourmeme.com/memes/press-f-to-pay-respects

Nightly 64.0a1 (2018-09-30) (64-bit)

Comment 4

[Benoit Girard (:BenWa)]

I ran a mozregression on it and it doesn't reproduce there, even if I reuse my profile. On my main profile it will reproduce even if I restart the browser and go to the URL immediately.

Comment 5

[Calixte Denizet (:calixte)]

The signature is ranked #1 in content top crashers for nightly

Comment 6

[Calixte Denizet (:calixte)]

[Tracking Requested - why for this release]:
We have around 10000 crashes on the last 7 days in nightly.

Comment 7

[Julien Cristau [:jcristau]]

I think we need a fix or a backout today.

Comment 8

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

As :gsnedders notes in comment 2, this is basically the same situation as bug 1487227.  Tracking protection induces asymmetries in the storage-allowed checks between channel and window.

This rush of crashes was due to turning on tracking protection by default on nightly in bug 1492563 and has ceased again in build 20180930220130 with the landing of the backout commit https://hg.mozilla.org/mozilla-central/rev/463e670ab9dd in https://bugzilla.mozilla.org/show_bug.cgi?id=1492563#c18

We can probably expect some level of crashes to continue for those who have explicitly enabled.

We can downgrade the diagnostic asserts but they are capturing an important invariant violation:
- The channel check said it's okay to use storage.
- The window check said it's not okay to use storage.

I'll see if there's a clean way for us to change the channel check into a window check.

Comment 9

[Sam Sneddon [:gsnedders]]

FWIW: I have repeatedly seen this crash with network.cookie.cookieBehavior;0 (i.e., the default), and I've never changed the pref.

Comment 10

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

(In reply to Geoffrey Sneddon [:gsnedders] from comment #9)
> FWIW: I have repeatedly seen this crash with network.cookie.cookieBehavior;0
> (i.e., the default), and I've never changed the pref.

Hm, that's more than a little concerning.  Are you sure you weren't impacted by the default being changed on nightly for a few days?  (If your setting was the default of 0, then when the default was changed you would end up using the new default unless you explicitly changed the setting back to 0 when the default value was changed on nightly.)

If you could share some of your crash-ids from "about:crashes" that would be useful.  You can send them to me privately at [e-mail].

Related questions might be if you use any WebExtensions that might do anything that would intercept requests with the webRequest API or use the cookies' APIs to automatically set the cookie preferences on a per-site basis.  Thanks!

Comment 12

[Sam Sneddon [:gsnedders]]

(In reply to Andrew Sutherland [:asuth] from comment #10)
> (In reply to Geoffrey Sneddon [:gsnedders] from comment #9)
> > FWIW: I have repeatedly seen this crash with network.cookie.cookieBehavior;0
> > (i.e., the default), and I've never changed the pref.
> 
> Hm, that's more than a little concerning.  Are you sure you weren't impacted
> by the default being changed on nightly for a few days?  (If your setting
> was the default of 0, then when the default was changed you would end up
> using the new default unless you explicitly changed the setting back to 0
> when the default value was changed on nightly.)
> 
> If you could share some of your crash-ids from "about:crashes" that would be
> useful.  You can send them to me privately at [e-mail].
> 
> Related questions might be if you use any WebExtensions that might do
> anything that would intercept requests with the webRequest API or use the
> cookies' APIs to automatically set the cookie preferences on a per-site
> basis.  Thanks!

So the pref will have got changed when it was the default, but I thought I'd actually had crashes *after* that got backed out. Looking again that doesn't seem to be the case (last crash in the last build before the back out). Hmm. No idea why I thought otherwise now!

Sorry for the noise!

Comment 13

[Stephen Moehle]

It looks like the change for bug 1492563 got pushed again, and now nightly (on Linux, at least) is crashing again as of build id 20181002220140.

Comment 14

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

Attachment: browser test to repro crash

Comment 15

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

Attachment: AntiTracking MOZ_LOG output from running with the test prior to the crash

I've attached the browser test that triggers the assertion, and here's the MOZ_LOG=AntiTracking:5 output from the segment of the test that triggers the actual assertion.

Comment 16

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a10b9f5fa82114d36166eede6ef48e0c76aee7f

Comment 17

[Andrea Marchesini [:baku]]

Attachment: patch

The idea behind this patch is:

1. When we need to check if a channel should be intercepted or not...
2. if the current prefs/permission require the channel to be marked as tracking resource...
3. let's create a dummy channel (TrackingDummyChannel) on the parent side
4. let's check if that dummy channel is a tracking resource
5. update the original HttpChannelChild
6. evaluate if we should intercept the channel again.

This introduces an extra jump on the parent process, but it happens only if cookieBahavior/permission needs it. All this code will go away when the interception will be done on the parent side only.

Comment 18

[François Marier [:francois]]

Comment on attachment 9014705 [details] [diff] [review]
patch

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

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ -324,5 @@
>    LOG(("HttpBaseChannel::SetIsTrackingResource thirdparty=%d %p",
>         static_cast<int>(aIsThirdParty), this));
>  
>    if (aIsThirdParty) {
> -    MOZ_ASSERT(!mIsFirstPartyTrackingResource);

Why do we need to remove these asserts?

They were added to ensure that we can't mark a channel as both a third-party tracker and a first-party tracker.

::: toolkit/components/antitracking/AntiTrackingCommon.cpp
@@ +1245,5 @@
>    cc->SendStoreUserInteractionAsPermission(IPC::Principal(aPrincipal));
>  }
> +
> +/* static */ bool
> +AntiTrackingCommon::ChannelNeedsToBeAnnotated(nsIHttpChannel* aChannel,

Are the annotations only needed for cookie restrictions?

What about the other users of tracking annotations? Aren't they going to be running into the same problem where intercepted channels are annotated too late?

Comment 19

[Andrea Marchesini [:baku]]

> > -    MOZ_ASSERT(!mIsFirstPartyTrackingResource);
> 
> Why do we need to remove these asserts?

It was left from an a previous version of the patch where I didn't store the 3rd/1st party information in the dummy channel.

> Are the annotations only needed for cookie restrictions?

For the purpose of this bug, yes, but we can generalize it to mark any channel, when should be intercepted, if the pref privacy.trackingprotection.annotate_channels is set to true.

Comment 20

[Andrea Marchesini [:baku]]

Attachment: sw_0.patch

Comment 21

[Andrea Marchesini [:baku]]

Attachment: sw_0.patch

Better approach in which I use the original nsILoadInfo in the dummy channel.

Comment 22

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

Comment on attachment 9014995 [details] [diff] [review]
sw_0.patch

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

This is a good solution to an ugly problem, thank you!  I propose some documentation and additional guards below.

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +2726,5 @@
> +                                        shouldUpgrade);
> +      intercepted->NotifyController();
> +    };
> +
> +    TrackingDummyChannel::StorageAllowedState state =

I think you should be bypassing the StorageAllowed check for cases for which nsContentUtils::IsNonSubresourceRequest() returns false (which for our current implementation purposes basically means a navigation request).  As you can see at the check for this scenario at https://searchfox.org/mozilla-central/rev/4b6737ba4008b71182cc2a12f507c82adf2b4e70/dom/serviceworkers/ServiceWorkerInterceptController.cpp#31, whether we attempt to fetch a sub-resource exclusively depends on whether or not there's a controller associated with the loadInfo (and conceptually, client).

Now, there is a nuance here.  Just because a client is controlled doesn't mean the channel will be intercepted.  If the SW has no 'fetch' event handler registered or doesn't respondWith() to the event, then the request will go to the parent.  And in that case we end up with the same potential issue again where in the client we didn't have the annotation but in the parent we did.  However, we don't care in this case because it's not actually impacting decision-making.  We only care for navigation requests.

And this is likely to matter for real-world purposes.  I understand via back-channels that the overhead of our no-fetch optimization case is already concerning to some sites.  This would add additional latency to that common scenario where a SW is registered for push notifications at the root scope.  (Which is arguably foolish, but I guess we encouraged sites to do that somehow?)

::: netwerk/protocol/http/PTrackingDummyChannel.ipdl
@@ +9,5 @@
> +
> +namespace mozilla {
> +namespace net {
> +
> +protocol PTrackingDummyChannel

Please add a block comment here or someplace else obvious in the patch that explains why the channel exists, when it can go away, etc.

Here's a possible start:

Provides a mechanism for the "child intercept" mode of ServiceWorker operation to work correctly with Tracking Protection annotations.  ServiceWorkers should not be allowed for third-party iframes which are annotated as tracking origins.

In child intercept mode, the decision to intercept a channel is made in the child process without consulting the parent process.  The decision is based on whether there is a ServiceWorker with a scope covering the URL in question and whether storage is allowed for the origin/URL.  When the "network.cookie.cookieBehavior" preference is set to BEHAVIOR_REJECT_TRACKER, annotated channels are denied storage which means that the ServiceWorker should not intercept the channel.  However, the decision for tracking protection to annotate a channel only occurs in the parent process.  The dummy channel is a hack to allow the intercept decision process to ask the parent process if the channel should be annotated.  Because this round-trip to the parent has overhead, the dummy channel is only created 1) if the ServiceWorker initially determines that the channel should be intercepted and 2) it's a navigation request.

This hack can be removed once Bug 1231208's new "parent intercept" mechanism fully lands, the pref is enabled by default it stays enabled for long enough to be confident we will never need/want to turn it off.  Then as part of bug 1496997 we can remove this implementation.  Bug TBD covers removing this hack in particular.

::: netwerk/protocol/http/TrackingDummyChannel.cpp
@@ +19,5 @@
> +
> +namespace {
> +
> +bool
> +ChannelNeedsToBeAnnotated()

Please either augment the logic here or add a comment here that explains the rationale behind checking only on "privacy.trackingprotection.annotate_channels" and when it would possibly be disabled.  It seems like that preference is going to be on all of the time per https://searchfox.org/mozilla-central/rev/4b6737ba4008b71182cc2a12f507c82adf2b4e70/modules/libpref/init/all.js#1366 setting it and no UI existing to set it to false (other than about:config).

In particular, it seems like the decision for storage being denied due to annotation is a product of:
1. The channel being annotated as a tracker, which is dependent on the "privacy.trackingprotection.annotate_channels" pref.
2. The channel being associated with an iframe which is a context thing.
3. The cookie behavior for the origin being set to BEHAVIOR_REJECT_TRACKER=4 which is dependent on the global "network.cookie.cookieBehavior" pref and the "cookie" permission as tracked by nsIPermissionManager.

Without a comment it's not clear why the 3rd case is not being factored into this decision given that the cookie behavior preference is available globally and the ServiceWorkerManagerService propagates the permissions for the origin to the content process at https://searchfox.org/mozilla-central/rev/4b6737ba4008b71182cc2a12f507c82adf2b4e70/dom/serviceworkers/ServiceWorkerManagerService.cpp#122 in addition to the preload opportunity at https://searchfox.org/mozilla-central/rev/4b6737ba4008b71182cc2a12f507c82adf2b4e70/dom/ipc/ContentParent.cpp#5571 also propagating the permissions.

::: netwerk/protocol/http/TrackingDummyChannelParent.cpp
@@ +25,5 @@
> +                                 nsILoadInfo* aLoadInfo)
> +{
> +  MOZ_ASSERT(mIPCActive);
> +
> +  RefPtr<TrackingDummyChannelParent> self =this;

nit: missing whitespace

Comment 23

[François Marier [:francois]]

Comment on attachment 9014995 [details] [diff] [review]
sw_0.patch

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

The parts that have to do with the classification look good to me, but please ask a Necko peer to review as well.

Comment 24

[Honza Bambas (:mayhemer)]

Comment on attachment 9014995 [details] [diff] [review]
sw_0.patch

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

r+ with comments addressed

please, please, please..  start adding comments to your patches :)  it really is important and makes all our lives (including yours!) better.  thanks.

::: dom/serviceworkers/test/browser_antitracking.js
@@ +77,5 @@
>        return payload;
>      }
>    );
>  
> +  ok(!controlled, "Should not be controlled!");

this is actually demonstrating the fix? :)

::: netwerk/protocol/http/TrackingDummyChannel.h
@@ +19,5 @@
> +
> +namespace mozilla {
> +namespace net {
> +
> +class TrackingDummyChannel final : public nsIChannel

add comments where and how this class is used

::: netwerk/protocol/http/TrackingDummyChannelChild.cpp
@@ +25,5 @@
> +    return false;
> +  }
> +
> +  URIParams uriParams;
> +  SerializeURI(aURI, uriParams);

I heard rumors we can send uris directly now

@@ +78,5 @@
> +
> +  bool storageGranted =
> +    AntiTrackingCommon::IsFirstPartyStorageAccessGrantedFor(httpChannel, mURI,
> +                                                            nullptr);
> +  mCallback(storageGranted);

are there any calling thread requirements?

::: netwerk/protocol/http/TrackingDummyChannelParent.cpp
@@ +42,5 @@
> +    new nsChannelClassifier(channel);
> +
> +  bool willCallback =
> +    NS_SUCCEEDED(channelClassifier->CheckIsTrackerWithLocalTable(
> +      [self, channel]() {

these days you can use self{std::move(self)}

::: toolkit/components/antitracking/AntiTrackingCommon.cpp
@@ -769,5 @@
>  
> -  if (NS_WARN_IF(NS_FAILED(rv) || !channelPrincipal)) {
> -    LOG(("No channel principal, bail out early"));
> -    return false;
> -  }

I don't think this change is correct.  we use the information of channelPrincipal == nullptr to find the toplevelPrincipal.  

if you believe this change is correct, then add a good comment why it's fine to be moved.

Comment 25

[Andrea Marchesini [:baku]]

Comments added. Mainly taken from the asuth's comment #22.

> are there any calling thread requirements?

Assertions Added. Yes, this is main-thread only.

> > -  if (NS_WARN_IF(NS_FAILED(rv) || !channelPrincipal)) {
> 
> I don't think this change is correct.  we use the information of
> channelPrincipal == nullptr to find the toplevelPrincipal.  

reverted. It was a completely unrelated change.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6d88f69e849
browser test to check how ServiceWorkers intercept tracking resources, r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/75ed7377db61
Introduce TrackingDummyChannel to annotate channels before being intercepted by ServiceWorkers, r=francois, r=mayhemer, f=asuth

Comment 27

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/f6d88f69e849
https://hg.mozilla.org/mozilla-central/rev/75ed7377db61

Comment 28

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

Tracking for 63. The crash volume on 63 beta is low but if we have a dot release in between 63 and 64, it might be a candidate for a ride-along uplift depending on the volume of crashes we get on the release channel.

Comment 29

[Marcia Knous [:marcia]]

So it appears we still have crashes on nightly after this patch landed, and a spike in the 10-17 build (116 crashes) - See Bug 1499995 where a user is crashing on theverge.com site. Should I reopen this or track this in Bug 1499995?

Comment 30

[Andrea Marchesini [:baku]]

Let's work on the other bug. It seems that the fix for this bug has reduced the number of crashes significantly. But something is still there. I prefer to work on a new fresh bug.