Closed
Bug 1221279
Opened 5 years ago
Closed 5 years ago
Crash in imgLoader cleanup when closing controlled window
Categories
(Core :: DOM: Service Workers, defect)
Core
DOM: Service Workers
Tracking
()
RESOLVED
FIXED
mozilla45
People
(Reporter: bkelly, Assigned: bkelly)
References
Details
Attachments
(1 file, 2 obsolete files)
1.97 KB,
patch
|
smaug
:
review+
ritu
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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++
Assignee | ||
Comment 1•5 years ago
|
||
Sorry, I meant nsDocument::Destroy() expected imgLoader to always be non-null.
Assignee | ||
Comment 2•5 years ago
|
||
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)
Updated•5 years ago
|
Attachment #8682727 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 3•5 years ago
|
||
Better patch that addresses XXX comment.
Attachment #8682727 -
Attachment is obsolete: true
Attachment #8682738 -
Flags: review?(bugs)
Assignee | ||
Comment 4•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dfcdbb7e6246
Assignee | ||
Comment 5•5 years ago
|
||
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)
Updated•5 years ago
|
Attachment #8682753 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 7•5 years ago
|
||
Bug 1202085 was uplifted to b2g 2.5 this morning, so its now affected by this as well.
Assignee | ||
Comment 8•5 years ago
|
||
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?
Comment 9•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f269d1d89973
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Assignee | ||
Comment 10•5 years ago
|
||
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?
Assignee | ||
Comment 11•5 years ago
|
||
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.
Attachment #8682753 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 13•5 years ago
|
||
(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•5 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/3f5340e348d4
Assignee | ||
Comment 15•5 years ago
|
||
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?
Comment 16•5 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/3f5340e348d4
You need to log in
before you can comment on or make changes to this bug.
Description
•