1133242 - trained-to-thrill Service Worker crashes child process on FirefoxOS

Status: RESOLVED FIXED

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

Description

[Ben Kelly [:bkelly, not reviewing]]

Using this patch queue (build available soon):

  https://github.com/wanderview/gecko-patches/tree/serviceworkers

I see the child process crash when visiting trained-to-thrill:

  https://jakearchibald.github.io/trained-to-thrill/

Note, it happens on the second visit when fetch events start getting processed.

This could be related to bug 1133238 where we see a CORS failure, but I'm not sure.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Is this something the Paris folks have already solved?  Am I missing a patch from that queue?

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

This seems to effect mdn.github.io/sw-test as well.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

I didn't see anything in logcat.  Maybe a debug build would show more.  Unfortunately I don't have time to investigate now.

Comment 4

[Fernando Jiménez Moreno [:ferjm]]

I am hitting this crash as well. I can see this in the logcat:

 208  Gecko  I  [Parent 208] WARNING: Principal is invalid, killing app process: file ../../../dom/ipc/AppProcessChecker.cpp, line 222
 208  Gecko  I  [Parent 208] WARNING: 'mActor && !AssertAppPrincipal(mActor, principal)', file ../../../dom/cache/PrincipalVerifier.cpp, line 137
 208  Gecko  I  [Parent 208] WARNING: 'NS_FAILED(aRv)', file ../../../dom/cache/CacheStorageParent.cpp, line 225

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Thanks Fernando!  I wonder if the appId being passed is garbage or something.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

I believe Fernando and Andrea are investigating this.  It does seem the principal is incorrect.  They tell me the security check is seeing:

  appId 0 and inBrowserElement false

But logcat indicates the browser has these values instead:

  AppId 32 inBrowserElement 1

So the ServiceWorker is getting the wrong principal somehow.

Comment 7

[Fernando Jiménez Moreno [:ferjm]]

Some more information. It seems that the principal is wrong when we get to WorkerGlobalScope::GetCaches which seems to be the starting point for the Cache API.

30657 Gecko  I  ServiceWorkerManager::Register. documentPrincipal. appId 32 inBrowser 1
30657 Gecko  I  ServiceWorkerRegisterJob::Start. principal. appId 32
30657 Gecko  I  ServiceWorkerRegisterJob::Start mJobType == REGISTER_JOB
30657 Gecko  I  ServiceWorkerRegisterJob::Start mRegistration
30657 Gecko  I  StoreRegistration 32
30657 Gecko  I  PopulateRegistrationData appId 32
29882 Gecko  I  AppId 32 inBrowserElement 1
29882 Gecko  I  OwnOrContainingAppId 32

30657 Gecko  I  WorkerGlobalScope::GetCaches appId 0
30657 Gecko  I  CacheStorage::CreateOnWorker
30657 Gecko  I  CacheStorage::CacheStorage
30657 Gecko  I  CacheStorage::ActorCreated
29882 Gecko  I  CacheStorageParent::CacheStorageParent appId 0
30657 Gecko  I  CacheStorage::Open
29882 Gecko  I  VerifyOnMainThread()
29882 Gecko  I  AppId 0 inBrowserElement 0
29882 Gecko  I  OwnOrContainingAppId 32
29882 Gecko  I  [Parent 29882] WARNING: Principal is invalid, killing app process: file ../../../dom/ipc/AppProcessChecker.cpp, line 224
29882 Gecko  I  [Parent 29882] WARNING: 'mActor && !AssertAppPrincipal(mActor, principal)', file ../../../dom/cache/PrincipalVerifier.cpp, line 138
29882 Gecko  I  [Parent 29882] WARNING: 'NS_FAILED(aRv)', file ../../../dom/cache/CacheStorageParent.cpp, line 228

Comment 8

[Fernando Jiménez Moreno [:ferjm]]

I can also reproduce this issue with http://ferjm.github.io/bug1133242/index.html The sw code is at https://github.com/ferjm/bug1133242/blob/master/sw.js

Comment 9

[Andrea Marchesini [:baku]]

https://mxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerManager.cpp#2182

This is the reason why we have this crash.

Comment 10

[Andrea Marchesini [:baku]]

Attachment: principal.patch

Comment 11

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8565154 [details] [diff] [review]
principal.patch

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

Have you verified that this fixes the problem?

Comment 12

[Fernando Jiménez Moreno [:ferjm]]

FWIW the patch fixes the issue for me. Thank you Andrea!

Comment 13

[Andrea Marchesini [:baku]]

Attachment: 229391.diff

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

Comment 14

[Andrea Marchesini [:baku]]

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

Comment 15

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/32e74ad032ea

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/32e74ad032ea