1221279 - Crash in imgLoader cleanup when closing controlled window

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

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++

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Don't crash while clearing imgLoader cache when disconnected document is destroyed. r=smaug

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

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Don't crash while clearing imgLoader cache when disconnected document is destro yed. r=smaug

Better patch that addresses XXX comment.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Don't crash while clearing imgLoader cache when disconnected document is destroyed. r=smaug

Sorry, mercurial made the patch a bit harder to read than it needed to.

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

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f269d1d89973

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

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 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f269d1d89973

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Bug 1202085 was uplifted to aurora.

Comment 12

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 13

[Carsten Book [:Tomcat]]

(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 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3f5340e348d4

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/3f5340e348d4