While working on something else I discovered the following crash. The problem is that nsContentUtils::GetImgLoaderForDocument() can return nullptr here: https://dxr.mozilla.org/mozilla-central/rev/59a6ad6a921f4809dfc37d943d765300c65721e5/dom/base/nsContentUtils.cpp#3098 But the new imgLoader::ClearCacheForControlledDocument() expects it to always return non-null. Crash stack: xul.dll!PLDHashTable::EntryStore::Get() Line 236 C++ xul.dll!PLDHashTable::Iterator::Iterator(PLDHashTable * aTable) Line 753 C++ xul.dll!nsBaseHashtable<nsGenericHashKey<mozilla::image::ImageCacheKey>,RefPtr<imgCacheEntry>,imgCacheEntry * __ptr64>::Iterator::Iterator(nsBaseHashtable<nsGenericHashKey<mozilla::image::ImageCacheKey>,RefPtr<imgCacheEntry>,imgCacheEntry *> * aTable) Line 249 C++ xul.dll!nsBaseHashtable<nsGenericHashKey<mozilla::image::ImageCacheKey>,RefPtr<imgCacheEntry>,imgCacheEntry * __ptr64>::Iter() Line 267 C++ xul.dll!imgLoader::ClearCacheForControlledDocument(nsIDocument * aDoc) Line 1370 C++ > xul.dll!nsDocument::Destroy() Line 8904 C++ xul.dll!nsDocumentViewer::Destroy() Line 1686 C++ xul.dll!`anonymous namespace'::EvictContentViewerForTransaction(nsISHTransaction * aTrans) Line 228 C++ xul.dll!nsSHistory::EvictAllContentViewers() Line 806 C++ xul.dll!nsDocShell::Destroy() Line 5685 C++

Sorry, I meant nsDocument::Destroy() expected imgLoader to always be non-null.

I'll file a follow-up for the XXX comment. I may need Ehsan's opinion on that when he gets back from PTO.

Better patch that addresses XXX comment.

Sorry, mercurial made the patch a bit harder to read than it needed to. https://treeherder.mozilla.org/#/jobs?repo=try&revision=b1cdd41a6eae

Bug 1202085 was uplifted to b2g 2.5 this morning, so its now affected by this as well.

Comment on attachment 8682753 [details] [diff] [review] Don't crash while clearing imgLoader cache when disconnected document is destroyed. r=smaug NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings. [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 1202085 which was uplifted to b2g44 v2.5 User impact if declined: Crash when closing some service worker controlled windows. Testing completed: Local testing. Existing mochitests ensure no functional regressions. Risk to taking this patch (and alternatives if risky): Minimal. It only affects service workers. String or UUID changes made by this patch: None

Comment on attachment 8682753 [details] [diff] [review] Don't crash while clearing imgLoader cache when disconnected document is destroyed. r=smaug Approval Request Comment [Feature/regressing bug #]: Bug 1202085 [User impact if declined]: Crashes when service workers are enabled. [Describe test coverage new/current, TreeHerder]: Existing mochitests in bug 1202085 check functional behavior. [Risks and why]: Minimal. Only affects service workers. [String/UUID change made/needed]: None.

Bug 1202085 was uplifted to aurora.

Comment on attachment 8682753 [details] [diff] [review] Don't crash while clearing imgLoader cache when disconnected document is destroyed. r=smaug It seems to me this fix is needed since we also took the fix from bug 1202085 in to Aurora44.

(In reply to Ben Kelly [:bkelly] from comment #8) > Comment on attachment 8682753 [details] [diff] [review] > Don't crash while clearing imgLoader cache when disconnected document is > destroyed. r=smaug > > NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to > better understand the B2G approval process and landings. > > [Approval Request Comment] > Bug caused by (feature/regressing bug #): bug 1202085 which was uplifted to > b2g44 v2.5 > User impact if declined: Crash when closing some service worker controlled > windows. > Testing completed: Local testing. Existing mochitests ensure no functional > regressions. > Risk to taking this patch (and alternatives if risky): Minimal. It only > affects service workers. > String or UUID changes made by this patch: None Hey Ben, currently aurora is merged every European Morning by me to b2g44 so since i work on uplifting this to aurora since it has ritu's approval it will land the next morning on b2g44 :)

Comment on attachment 8682753 [details] [diff] [review] Don't crash while clearing imgLoader cache when disconnected document is destroyed. r=smaug Dropping flag per comment 13.