1329759 - Crash in nsTHashtable<T>::s_HashKey | PLDHashTable::Add | nsClassHashtable<T>::LookupOrAdd<T> | mozilla::dom::workers::ServiceWorkerManager::AddNavigationInterception

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

This bug was filed from the Socorro interface and is 
report bp-20f00833-fdf6-47c5-9d45-19e952161215.
=============================================================

I believe this is happening because we are getting a nullptr active service worker in some circumstances here:

https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerManager.cpp#2398

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Gracefully handle a missing active service worker during interception. r=asuth

I changed the assertion to be a diagnostic since we really shouldn't get a nullptr here.  But at lets at least avoid crashing if we can.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8825153 [details] [diff] [review]
Gracefully handle a missing active service worker during interception. r=asuth

I need to change a few more things here.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Gracefully handle a missing active service worker during interception. r=asuth

Updated based on our irc conversation.  It is possible for the active worker to be nullptr here since there is time between IsAvailable() and DispatchFetchEvent().

I also made the code return an error code if there is no service worker.  This is necessary to properly restart the original network request:

https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/InterceptedChannel.cpp#81

Comment 4

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

Comment on attachment 8825155 [details] [diff] [review]
Gracefully handle a missing active service worker during interception. r=asuth

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

Have I mentioned how much I love it that you add informative comments when making changes?  Thank you!

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/15057def8d40
Gracefully handle a missing active service worker during interception. r=asuth

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/15057def8d40

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8825155 [details] [diff] [review]
Gracefully handle a missing active service worker during interception. r=asuth

Approval Request Comment
[Feature/Bug causing the regression]: Service workers
[User impact if declined]: Around 40 crashes per week in release channel.
[Is this code covered by automated tests?]: The code is covered by extensive automated tests, but none of them hit the corner case that triggered this crash.
[Has the fix been verified in Nightly?]: We don't have exact steps to reproduce.  Its been in nightly for a day or so.
[Needs manual test from QE? If yes, steps to reproduce]:  None
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Minimal
[Why is the change risky/not risky?]: The patch basically adds some additional nullptr checks and diagnostic assertions.
[String changes made/needed]: None

Comment 8

[Gerry Chang [:gchang]]

Comment on attachment 8825155 [details] [diff] [review]
Gracefully handle a missing active service worker during interception. r=asuth

Fix a crash. Beta51+ & Aurora52+. Should be 51 beta 14.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/52c0acda762192a43cc09dd72f808d0ad3bd32da

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e7fb3b64ad2df6f3ad035ab17fc9c8444bce6af6