Crash due to MOZ_RELEASE_ASSERT in nsDocumentViewer::~nsDocumentViewer

RESOLVED FIXED in Firefox -esr52

Status

()

defect
P1
normal
RESOLVED FIXED
a year ago
8 months ago

People

(Reporter: bobowen, Assigned: bobowen)

Tracking

({sec-audit})

59 Branch
mozilla59
Points:
---

Firefox Tracking Flags

(firefox-esr5259+ fixed, firefox58+ fixed, firefox59 fixed)

Details

(Whiteboard: [adv-esr52.7-], crash signature)

Attachments

(1 attachment)

(Assignee)

Description

a year ago
We're hitting the MOZ_RELEASE_ASSERT added for bug 1261175, so this means that we must not be decrementing mDestroyRefCount when the nsPagePrintTimer dies.

Moving the decrement to its own function should ensure this.
Group: core-security → layout-core-security
Keywords: sec-audit
(In reply to Bob Owen (:bobowen) from comment #0)
> We're hitting the MOZ_RELEASE_ASSERT added for bug 1261175, so this means
> that we must not be decrementing mDestroyRefCount when the nsPagePrintTimer
> dies.

Presumably because we hit the early |mPrintJob->CheckBeforeDestroy()| return in nsDocumentViewer::Destroy().
I tried to come up with a description of the lifetimes and ownership between nsDocumentViewer, nsPrintJob, nsPagePrintTimer, all the callbacks/timers it creates, etc., but the deeper I dug the more insane it got. Eventually I gave up. We really need to refactor this code and get a handle on it!
Comment on attachment 8937682 [details] [diff] [review]
Decrement nsDocumentViewer::mDestroyRefCount in separate function

Review of attachment 8937682 [details] [diff] [review]:
-----------------------------------------------------------------

I'd note that prior to this patch nsPagePrintTimer's call to nsDocumentViewer::IncrementDestroyRefCount() would block a single nsDocumentViewer::Destroy() call. In principle we will now block all Destroy() calls (from all the many places they can come from) until the nsPagePrintTimer dies. That would seem to be as desired, but with this code in its current state who knows...

::: layout/printing/nsPagePrintTimer.cpp
@@ -19,5 @@
>  {
> -  // "Destroy" the document viewer; this normally doesn't actually
> -  // destroy it because of the IncrementDestroyRefCount call below
> -  // XXX This is messy; the document viewer should use a single approach
> -  // to keep itself alive during printing

No kidding!!
Attachment #8937682 - Flags: review?(jwatt) → review+
(Assignee)

Comment 5

a year ago
Thanks.

(In reply to Jonathan Watt [:jwatt] (needinfo? me) from comment #2)
> (In reply to Bob Owen (:bobowen) from comment #0)
> > We're hitting the MOZ_RELEASE_ASSERT added for bug 1261175, so this means
> > that we must not be decrementing mDestroyRefCount when the nsPagePrintTimer
> > dies.
> 
> Presumably because we hit the early |mPrintJob->CheckBeforeDestroy()| return
> in nsDocumentViewer::Destroy().

Yes, this was the only way I could see (bug 1261175 comment 80), although even armed with that knowledge I couldn't reproduce.

(In reply to Jonathan Watt [:jwatt] (needinfo? me) from comment #3)
> I tried to come up with a description of the lifetimes and ownership between
> nsDocumentViewer, nsPrintJob, nsPagePrintTimer, all the callbacks/timers it
> creates, etc., but the deeper I dug the more insane it got. Eventually I
> gave up. We really need to refactor this code and get a handle on it!

Then there's also nsPrintObject and nsPrintData, which seem to be poorly defined abstractions and nsPrintJob messes directly with their data, sometimes passing it to other objects.

(In reply to Jonathan Watt [:jwatt] (needinfo? me) from comment #4)
...
> I'd note that prior to this patch nsPagePrintTimer's call to
> nsDocumentViewer::IncrementDestroyRefCount() would block a single
> nsDocumentViewer::Destroy() call. In principle we will now block all
> Destroy() calls (from all the many places they can come from) until the
> nsPagePrintTimer dies. That would seem to be as desired, but with this code
> in its current state who knows...

Yes and this makes me a bit nervous about landing this late in the cycle.
I did wonder whether I should wait for Fx60 and then uplift, but then we have less time Beta (and overall) for any problems to surface.
However, given that we think that all of the crashes in bug 1261175 are going to morph into this one in Beta 59, I think on balance landing now is the lesser of the two risks.
Crash Signature: [@ nsDocumentViewer::~nsDocumentViewer]
(Assignee)

Comment 8

a year ago
Relaying overholt's request from the not hidden version of this bug (bug 1431009).

jwatt - I haven't seen any fallout from this, although I'm not sure I'd know it when I saw it. It certainly seems to have fixed the crash. Do you think this is safe to uplift?
Flags: needinfo?(jwatt)
I think it should be. My main concern when I was looking hard at this code was that there might be circumstances when we might end up leaking print documents. I pretty much convinced myself that this change wouldn't introduce any new leaks though. Regardless, even if that were the case it's probably better to leak the documents that a user prints under rare circumstances than it is to crash in a way that doesn't even help us diagnose any such leaks.
Flags: needinfo?(jwatt)
(Assignee)

Comment 10

a year ago
Comment on attachment 8937682 [details] [diff] [review]
Decrement nsDocumentViewer::mDestroyRefCount in separate function

(In reply to Jonathan Watt [:jwatt] (needinfo? me) from comment #9)
Thanks.

Approval Request Comment
[Feature/Bug causing the regression]:
In theory this was introduced by bug 1261175 making that crash safe, but the original crash is probably very old.

[User impact if declined]:
Crash of their content process when they hit this problem during printing.

[Is this code covered by automated tests?]:
No.

[Has the fix been verified in Nightly?]:
We don't have STR, but there have been no instances of the crash in Nightly 60 or Beta 59 (since the patch made it into the beta releases).

[Needs manual test from QE? If yes, steps to reproduce]:
No STR unfortunately.

[List of other uplifts needed for the feature/fix]:
None.

[Is the change risky?]:
Slightly.

[Why is the change risky/not risky?]:
The change itself is very simple and in fact now means the code does what the comments suggest it is supposed to do in all circumstances. Changing to what appears to be the correct/intended behaviour could have unforeseen consequences though.
However, this is a fairly common crash, so the benefits probably outweigh the risks.

[String changes made/needed]:
None
Attachment #8937682 - Flags: approval-mozilla-release?
Why is this bug not marked "FIXED" if the patch landed on central a couple weeks ago (comment 7)?
(Assignee)

Comment 12

a year ago
(In reply to Daniel Veditz [:dveditz] from comment #11)
> Why is this bug not marked "FIXED" if the patch landed on central a couple
> weeks ago (comment 7)?

Just an omission on the merge possibly.
aryx may well have been waiting to see if this actually fixed it, I think we can safely say that now.
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Comment on attachment 8937682 [details] [diff] [review]
Decrement nsDocumentViewer::mDestroyRefCount in separate function

DVeditz helped provide a second opinion on this one, seems like a safe ride-along, Release58+
Attachment #8937682 - Flags: approval-mozilla-release? → approval-mozilla-release+
(Assignee)

Comment 15

a year ago
Mike - this is the duplicate of Bug 1431009, that I don't want to duplicate to yet as it's related to a sec bug.
I was going to do it once 58 was pretty much fully ramped up, because the fix for the original bug is already in there.
This fix is in 58.0.2 whenever that goes out.
Given that the second patch from bug 1261175 was uplifted to ESR52, does this need an approval request as well?
Flags: needinfo?(bobowencode)
(Assignee)

Comment 17

a year ago
Comment on attachment 8937682 [details] [diff] [review]
Decrement nsDocumentViewer::mDestroyRefCount in separate function

(In reply to Ryan VanderMeulen [:RyanVM] from comment #16)
> Given that the second patch from bug 1261175 was uplifted to ESR52, does
> this need an approval request as well?

Yes makes sense.
Flags: needinfo?(bobowencode)
Attachment #8937682 - Flags: approval-mozilla-esr52?
Comment on attachment 8937682 [details] [diff] [review]
Decrement nsDocumentViewer::mDestroyRefCount in separate function

Taking for ESR 52.7 as well.
Attachment #8937682 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Al, just FYI, this was included in 58.0.2 to take care of a frequent crash (bug 1431009). I'm taking it on ESR52 as well to ride to 52.7 along with the recent uplift of bug 1261175.
Flags: needinfo?(abillings)
(In reply to Ryan VanderMeulen [:RyanVM] from comment #19)
> Al, just FYI, this was included in 58.0.2 to take care of a frequent crash
> (bug 1431009). I'm taking it on ESR52 as well to ride to 52.7 along with the
> recent uplift of bug 1261175.

Sec-audit so I'm cool with that if release management wanted it.
Flags: needinfo?(abillings)
Group: layout-core-security → core-security-release
Whiteboard: [adv-est52.7-]
Whiteboard: [adv-est52.7-] → [adv-esr52.7-]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.