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

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[baffclan]

This bug was filed from the Socorro interface and is
report bp-c273d470-a599-4a3b-b508-812eb0171224.
=============================================================

Top 10 frames of crashing thread:

0 xul.dll mozilla::dom::ClientSource::WindowExecutionReady dom/clients/manager/ClientSource.cpp:252
1 xul.dll nsGlobalWindowInner::ExecutionReady dom/base/nsGlobalWindowInner.cpp:1888
2 xul.dll nsGlobalWindowOuter::SetNewDocument dom/base/nsGlobalWindowOuter.cpp:2013
3 xul.dll nsDocumentViewer::InitInternal layout/base/nsDocumentViewer.cpp:934
4 xul.dll nsDocumentViewer::Init layout/base/nsDocumentViewer.cpp:665
5 xul.dll nsDocShell::SetupNewViewer docshell/base/nsDocShell.cpp:9720
6 xul.dll nsDocShell::Embed docshell/base/nsDocShell.cpp:7537
7 xul.dll nsDocShell::CreateContentViewer docshell/base/nsDocShell.cpp:9524
8 xul.dll nsDSURIContentListener::DoContent docshell/base/nsDSURIContentListener.cpp:196
9 xul.dll nsDocumentOpenInfo::TryContentListener uriloader/base/nsURILoader.cpp:739

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

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

The change that caused this was backed out.

Comment 2

[baffclan]

(In reply to Ben Kelly [:bkelly] from comment #1)
> The change that caused this was backed out.

regression by bug 1425975?

Comment 3

[baffclan]

WFM with Newest Nightly.

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:59.0) Gecko/20100101 Firefox/59.0
Build ID: 20171224100317

Comment 4

[Bryan Price]

It's baaaaack!

https://crash-stats.mozilla.com/report/index/86f407d5-de58-4cab-8dbf-1442d0180107

Worse, it seems to be an assertion causing the problem. :(

MOZ_RELEASE_ASSERT(nsContentUtils::StorageAllowedForWindow(aInnerWindow) == nsContentUtils::StorageAccess::eAllow)

This has made theoldreader.com pretty useless.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

If you have reliable steps to reproduce, that would be great.  Thanks.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Bryan, what add-ons and cookie settings are you using?

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Or have you denied storage permission explicitly for the site that is crashing?

Comment 8

[baffclan]

It is "Cookies setting".
e.g.,
https://twitter.com/ : Allow for Session
Accept third-party cookies : Never

STR: 
Set a cookies setting.
Open https://twitter.com/
Crash.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Ok, its per-site session cookie settings.  I can reproduce.  Unfortunately we cannot support blocking the first service worker fetch event based on those settings until we move to parent-side interception and fix bug 1428130.

For now I will at least make it not crash.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Preload the cookie permission to properly block client-side service worker interception. r=mystor

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Add StorageAllowedForNewWindow() to support docshell service worker checks. r=mystor

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Use StorageAllowedForNewWindow() in nsDocShell::ServiceWorkerAllowedToControlWindow(). r=mystor

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e1e10531e28eb1c13124299b658bf9548188f7f

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Add mochitest verifying service workers respect per-site cookie permissions. r=mystor

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

The test in P4 triggers the crash.  Patches P1, P2, and P3 allow us to pass the test while correctly blocking service workers.  There is some debate about this approach, though.  I'm not sure if I've convinced Nika if its ok or not yet, but lets see if we can go this way.  I would prefer to correctly respect the permission even if the preload is not ideal.

I'm going to wait for try to be green and then flag for review.

Comment 16

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8940752 [details] [diff] [review]
P2 Add StorageAllowedForNewWindow() to support docshell service worker checks. r=mystor

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

::: dom/base/nsContentUtils.h
@@ +2984,5 @@
> +   * Checks if storage should be allowed for a new window with the given
> +   * principal, load URI, and parent.
> +   */
> +  static StorageAccess StorageAllowedForNewWindow(nsIPrincipal* aPrincipal,
> +                                                  nsIURI* aURI,

I'm not sure why you need to have this as a separate parameter here rather than extracting it from the aPrincipal. It looks like in the one caller you add in p3 you always pass a codebase principal built from aURI to this function.

If its just a performance thing can you debug assert that the URI in the principal and aURI match?

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Add mochitest verifying service workers respect per-site cookie permissions. r=mystor

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8940751 [details] [diff] [review]
P1 Preload the cookie permission to properly block client-side service worker interception. r=mystor

Nika, this preloads the cookie permission as we discussed.  I have bug 1428130 filed to remove the preload once service worker begins checking storage in the parent process.

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8940752 [details] [diff] [review]
P2 Add StorageAllowedForNewWindow() to support docshell service worker checks. r=mystor

This patch adds another StorageAllowed*() helper routine for docshell.  Bsaically this just means treating the passed window as a parent window and providing an explicit URI for the window to be loaded.


Note, we require a separate URI to be passed since the URL can be things like about:blank and about:srcdoc which does not directly match the URI in the principal.

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8940753 [details] [diff] [review]
P3 Use StorageAllowedForNewWindow() in nsDocShell::ServiceWorkerAllowedToControlWindow(). r=mystor

This patch uses the new StorageAllowedForNewWindow() helper in docshell.

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8940800 [details] [diff] [review]
P4 Add mochitest verifying service workers respect per-site cookie permissions. r=mystor

Nika, this patch contains a test which verifies service worker is blocked when a per-site cookie permission is set.  Without any other patches this triggers the crash from comment 0.  With patches P1 to P3 the test passes.

Note, try did show some --verify failures, but I have not been able to reproduce them locally.  I did fix up a few more service worker related things in the test, but can't test in automation yet since taskcluster is down.  I think the test should be pretty solid now.

Comment 22

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Remove unnused cookie pref getters from nsContentUtils. r=mystor

And we can now nuke these cookie pref getters from nsContentUtils since docshell doesn't need them any more.

Comment 23

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8940751 [details] [diff] [review]
P1 Preload the cookie permission to properly block client-side service worker interception. r=mystor

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

Unfortunate but OK as a temporary solution. Thanks for filing the follow-up.

Comment 24

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8940800 [details] [diff] [review]
P4 Add mochitest verifying service workers respect per-site cookie permissions. r=mystor

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

::: dom/workers/test/serviceworkers/browser_storage_permission.js
@@ +42,5 @@
> +});
> +
> +add_task(async function test_allow_permission() {
> +  Services.perms.add(Services.io.newURI(PAGE_URI), "cookie",
> +                                        Ci.nsICookiePermission.ACCESS_ALLOW);

nit: indentation, here and in the other tests.

@@ +44,5 @@
> +add_task(async function test_allow_permission() {
> +  Services.perms.add(Services.io.newURI(PAGE_URI), "cookie",
> +                                        Ci.nsICookiePermission.ACCESS_ALLOW);
> +
> +  let tab = BrowserTestUtils.addTab(gBrowser, SCOPE);

It might be nice to factor these bits out into a helper, but it's not really a big deal so *shrug*.

Comment 25

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Add mochitest verifying service workers respect per-site cookie permissions. r=mystor

Comment 26

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e331e6fa1f38de146b0c327e782935a2eadd71e

Comment 27

[Ben Kelly [:bkelly, not reviewing]]

The TV failures are still there, but they are tier 2 so won't block landing.  I can't reproduce locally.

Comment 28

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e040e3e09aa916535941a3217818d6a0088feb2

Comment 29

[Ben Kelly [:bkelly, not reviewing]]

After looking at the debug I think the TV failure is a service worker cross-process propagation race.  I could change the test to use single process, but I'm not going to worry about it here because this problem does not manifest in the when normally run and our refactoring work will fix it in the long run.

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/11a02afca6fe
P1 Preload the cookie permission to properly block client-side service worker interception. r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/47a65c7932c7
P2 Add StorageAllowedForNewWindow() to support docshell service worker checks. r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b5c58cb8123
P3 Use StorageAllowedForNewWindow() in nsDocShell::ServiceWorkerAllowedToControlWindow(). r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab49425989fe
P4 Add mochitest verifying service workers respect per-site cookie permissions. r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/62603fdd915b
P5 Remove unnused cookie pref getters from nsContentUtils. r=mystor

Comment 31

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/11a02afca6fe
https://hg.mozilla.org/mozilla-central/rev/47a65c7932c7
https://hg.mozilla.org/mozilla-central/rev/5b5c58cb8123
https://hg.mozilla.org/mozilla-central/rev/ab49425989fe
https://hg.mozilla.org/mozilla-central/rev/62603fdd915b

Comment 32

[baffclan]

Cannot reproduce with newest Nightly.

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:59.0) Gecko/20100101 Firefox/59.0
Build ID: 20180109100117

Thanks for fixing this!

Comment 33

[baffclan]

Reproduce with newest nightly. 
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:60.0) Gecko/20100101 Firefox/60.0
Build ID: 20180126035135


This bug was filed from the Socorro interface and is
report bp-88f3c70a-9470-47fc-aef8-19b660180126.
=============================================================

Top 10 frames of crashing thread:

0 xul.dll mozilla::dom::ClientSource::WindowExecutionReady dom/clients/manager/ClientSource.cpp:252
1 xul.dll nsGlobalWindowInner::ExecutionReady dom/base/nsGlobalWindowInner.cpp:1922
2 xul.dll nsGlobalWindowOuter::SetNewDocument dom/base/nsGlobalWindowOuter.cpp:2013
3 xul.dll nsDocumentViewer::InitInternal layout/base/nsDocumentViewer.cpp:935
4 xul.dll nsDocumentViewer::Init layout/base/nsDocumentViewer.cpp:666
5 xul.dll nsDocShell::SetupNewViewer docshell/base/nsDocShell.cpp:9054
6 xul.dll nsDocShell::Embed docshell/base/nsDocShell.cpp:6876
7 xul.dll nsDocShell::CreateContentViewer docshell/base/nsDocShell.cpp:8856
8 xul.dll nsDSURIContentListener::DoContent docshell/base/nsDSURIContentListener.cpp:196
9 xul.dll nsDocumentOpenInfo::TryContentListener uriloader/base/nsURILoader.cpp:739

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

Comment 34

[Ben Kelly [:bkelly, not reviewing]]

Please file a new bug.

Comment 35

[baffclan]

(In reply to Ben Kelly [:bkelly] from comment #34)
> Please file a new bug.
Thanks for comment.
file a bug 1433454.