Closed Bug 1329759 Opened 3 years ago Closed 3 years ago

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

Categories

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

x86
Windows 10
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox-esr45 --- wontfix
firefox50 --- wontfix
firefox51 --- fixed
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: bkelly, Assigned: bkelly)

References

(Blocks 1 open bug)

Details

(Keywords: crash)

Crash Data

Attachments

(1 file, 1 obsolete file)

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
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.
Attachment #8825153 - Flags: review?(bugmail)
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.
Attachment #8825153 - Flags: review?(bugmail)
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
Attachment #8825153 - Attachment is obsolete: true
Attachment #8825155 - Flags: review?(bugmail)
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!
Attachment #8825155 - Flags: review?(bugmail) → review+
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/15057def8d40
Gracefully handle a missing active service worker during interception. r=asuth
https://hg.mozilla.org/mozilla-central/rev/15057def8d40
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
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
Attachment #8825155 - Flags: approval-mozilla-beta?
Attachment #8825155 - Flags: approval-mozilla-aurora?
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.
Attachment #8825155 - Flags: approval-mozilla-beta?
Attachment #8825155 - Flags: approval-mozilla-beta+
Attachment #8825155 - Flags: approval-mozilla-aurora?
Attachment #8825155 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.