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

RESOLVED FIXED in Firefox 35

Status

()

defect
RESOLVED FIXED
5 years ago
5 months ago

People

(Reporter: RyanVM, Assigned: johns)

Tracking

({crash, intermittent-failure})

Trunk
mozilla36
Points:
---
Dependency tree / graph

Firefox Tracking Flags

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

Details

Attachments

(1 attachment, 1 obsolete attachment)

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]
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: 5 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.
Please request aurora/beta/esr31 approval on this when you get a chance.
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.