Closed Bug 1221279 Opened 4 years ago Closed 4 years ago

Crash in imgLoader cleanup when closing controlled window

Categories

(Core :: DOM: Service Workers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox44 --- fixed
firefox45 --- fixed
b2g-v2.5 --- fixed

People

(Reporter: bkelly, Assigned: bkelly)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Attachment #8682727 - Flags: review?(bugs)
Attachment #8682727 - Flags: review?(bugs) → review+
Better patch that addresses XXX comment.
Attachment #8682727 - Attachment is obsolete: true
Attachment #8682738 - Flags: review?(bugs)
Sorry, mercurial made the patch a bit harder to read than it needed to.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b1cdd41a6eae
Attachment #8682738 - Attachment is obsolete: true
Attachment #8682738 - Flags: review?(bugs)
Attachment #8682753 - Flags: review?(bugs)
Attachment #8682753 - Flags: review?(bugs) → review+
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
Attachment #8682753 - Flags: approval‑mozilla‑b2g44?
https://hg.mozilla.org/mozilla-central/rev/f269d1d89973
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
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.
Attachment #8682753 - Flags: approval-mozilla-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.
Attachment #8682753 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(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.
Attachment #8682753 - Flags: approval‑mozilla‑b2g44?
You need to log in before you can comment on or make changes to this bug.