Closed Bug 1090530 Opened 10 years ago Closed 10 years ago

Intermittent test_img_mutations.html,test_picture_mutations.html | application terminated with exit code -9 after ASAN SEGV or application crashed [@ nsPresContext::AppUnitsPerDevPixel() const]

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla36
Tracking Flags:
Tracking Status
firefox34 --- wontfix
firefox35 --- fixed
firefox36 --- fixed
firefox-esr31 --- wontfix
b2g-v1.4 --- wontfix
b2g-v2.0 --- wontfix
b2g-v2.0M --- wontfix
b2g-v2.1 --- wontfix
b2g-v2.2 --- fixed

People

(Reporter: RyanVM, Assigned: johns)

References

Details

(Keywords: crash, intermittent-failure)

Attachments

(1 file, 1 obsolete file)

Ensure that we unregister preference observers in nsPresContext unlink. r=roc
5.03 KB, patch
johns
: review+
lsblakk
: approval-mozilla-aurora+
johns
: checkin+
Details | Diff | Splinter Review
No description provided.
Flags: needinfo?(jschoenick)
Per recent stars in bug 891840, test_picture_mutations.html is also affected.
Summary: Intermittent test_img_mutations.html | application terminated with exit code -9 after ASAN SEGV or application crashed [@ nsPresContext::AppUnitsPerDevPixel() const] → Intermittent test_img_mutations.html,test_picture_mutations.html | application terminated with exit code -9 after ASAN SEGV or application crashed [@ nsPresContext::AppUnitsPerDevPixel() const]
Blocks: srcset-prefon
Flags: needinfo?(jschoenick)
Assignee: nobody → jschoenick
Status: NEW → ASSIGNED
This is definitely not good, looking into
So the test is flipping layout.css.devPixelsPerPx which ends up in nsPresContext::PreferenceChanged, which calls nsPresContext::AppUnitsPerDevPixel, which crashes because mDeviceContext is (near?) null. But mDeviceContext is set in nsPresContext::Init, which also registers the preference observers, so nsPresContext->Init(nullptr) seems to be happening, or something else bad. I suspect its happening in this test because it's the first to fiddle with that pref
Also, only seems to have happened in e10s mode so far
Ugh, this is just a dupe of bug 891840, where other tests that touch this pref were disabled. Great.
It looks like the issue here is preference observers firing after unlink, at which point various class members are null'd. This factors out the bits common to unlink and the destructor to a Destroy function, and moves unregistering prefs there.
Attachment #8513788 - Flags: review?(roc)
Blocks: 891840
Comment on attachment 8513788 [details] [diff] [review] Ensure that we unregister preference observers in nsPresContext unlink Review of attachment 8513788 [details] [diff] [review]: ----------------------------------------------------------------- r+ with that ::: layout/base/nsPresContext.cpp @@ +258,4 @@ > if (mEventManager) { > // unclear if these are needed, but can't hurt > mEventManager->NotifyDestroyPresContext(this); > mEventManager->SetPresContext(nullptr); I think you should clear mEventManager here. @@ +313,5 @@ > + > + // Disconnect the refresh driver *after* the transition manager, which > + // needs it. > + if (mRefreshDriver && mRefreshDriver->PresContext() == this) { > + mRefreshDriver->Disconnect(); I think you should clear mRefreshDriver here.
Attachment #8513788 - Flags: review?(roc) → review+
Most recently affected suites for this bug and bug 891840 (w/ re-enable) https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4ac799fef00f
Comment on attachment 8513863 [details] [diff] [review] Ensure that we unregister preference observers in nsPresContext unlink. r=roc https://hg.mozilla.org/integration/mozilla-inbound/rev/5f1bda02950a
Attachment #8513863 - Flags: checkin+
(In reply to John Schoenick [:johns] from comment #22) > Comment on attachment 8513863 [details] [diff] [review] > Ensure that we unregister preference observers in nsPresContext unlink. r=roc > > https://hg.mozilla.org/integration/mozilla-inbound/rev/5f1bda02950a With the fun note that I pulled a bug 966545 when landing this, so it wont be query-able in the pushlog...
https://hg.mozilla.org/mozilla-central/rev/5f1bda02950a
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Given how ancient this bug is, I vote we backport this as far back as Release Management will let us.
status-firefox34: --- → affected
status-firefox35: --- → affected
status-firefox36: --- → fixed
status-firefox-esr31: --- → affected
Please request aurora/beta/esr31 approval on this when you get a chance.
status-b2g-v1.4: affected → wontfix
status-b2g-v2.0: affected → wontfix
status-b2g-v2.0M: affected → wontfix
Flags: needinfo?(jschoenick)
Comment on attachment 8513863 [details] [diff] [review] Ensure that we unregister preference observers in nsPresContext unlink. r=roc Requesting uplift to aurora for test coverage purposes, but given lack of evidence of this in the wild, leaving out beta/esr for now. Approval Request Comment [Feature/regressing bug #]: Longstanding [User impact if declined]: Lets us re-enable more test converage, possible rare (null deref) crash in the wild. [Describe test coverage new/current, TBPL]: Allows re-enabling bug 891840 tests, new tests on m-c also exercise this code. [Risks and why]: Low, small change that shuffles around existing teardown, unregisters preference observers that should not be touching unlinked object anyway. [String/UUID change made/needed]: None
Flags: needinfo?(jschoenick)
Attachment #8513863 - Flags: approval-mozilla-aurora?
Attachment #8513863 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: