Closed Bug 729401 Opened 9 years ago Closed 9 years ago

null deref in nsXULTemplateBuilder::Uninit

Categories

(Core :: XUL, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla13
Tracking Status
firefox-esr10 13+ verified

People

(Reporter: mccr8, Assigned: mccr8)

Details

Crash Data

Attachments

(1 file)

I noticed this crash in the crash reports.  Not a huge one, ranked around 193 on 10, but every single one is a null deref, being called from ~nsXULTemplateBuilder.

They are all showing up on the line with the RemoveObserver:
nsXULTemplateBuilder::Uninit(bool aIsFinal)
{
    if (mObservedDocument && aIsFinal) {
        gObserverService->RemoveObserver(this, DOM_WINDOW_DESTROYED_TOPIC);

Here's the code for the destructor:
    if (--gRefCnt == 0) {
        NS_IF_RELEASE(gRDFService);
        NS_IF_RELEASE(gRDFContainerUtils);
        NS_IF_RELEASE(gSystemPrincipal);
        NS_IF_RELEASE(gScriptSecurityManager);
        NS_IF_RELEASE(gObserverService);
    }

    Uninit(true);

Not knowing anything about this code, I would guess that maybe what is happening is that gObserverService is getting nulled out in the destructor, then uninit is attempting to deref it.
Attached patch WIPSplinter Review
untested, but from looking at the code it looks like this would do the trick.
This crash is currently ranked #207 in 10, #254 in 11, #232 in 13.  Doesn't show up in the top 300 in 12.  A grim threat to Firefox's stability!
Comment on attachment 599697 [details] [diff] [review]
WIP

I'm marking you for review because from HG blame it looks like maybe this was caused by a patch you wrote... in 2008.  Let me know if somebody else would be more appropriate.
Attachment #599697 - Flags: review?(bent.mozilla)
A try run for Linux and Mac looked good, so my patch isn't totally crazy.
Comment on attachment 599697 [details] [diff] [review]
WIP

I think this should be fine.
Attachment #599697 - Flags: review?(bent.mozilla) → review+
Thanks!

Try run was okay on Windows and Android.

https://hg.mozilla.org/integration/mozilla-inbound/rev/213f17d094fc
Assignee: nobody → continuation
Target Milestone: --- → mozilla13
https://hg.mozilla.org/mozilla-central/rev/213f17d094fc
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
It's #3 top crasher in 10.0.4 ESR.
That's really odd.  As I said in comment 0, when I first noticed this bug, release was 10, and this crash was ranked around #200.  I wonder what changed.
This looks like a shutdown-only crash, if that affects its severity.
The up times look a little low for a shutdown crash, so maybe the browser is exiting prematurely?
Please nominate for ESR approval so we can get this landed before the go to build next week.
Comment on attachment 599697 [details] [diff] [review]
WIP

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: This fixes the #3 crash on ESR, though I would suspect that there's some other issue causing this to spike up, which this won't fix.
User impact if declined: shutdown crashes
Fix Landed on Version: 13
Risk to taking this patch (and alternatives if risky): minor.  This only slightly reorders some code run at shutdown.  It has been in Firefox since 13 without any apparent changes.
String or UUID changes made by this patch: none
Attachment #599697 - Flags: approval-mozilla-esr10?
"without any apparent changes." I meant "apparent problems"
Comment on attachment 599697 [details] [diff] [review]
WIP

approving low-risk crash-fix.
Attachment #599697 - Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
I'm still seeing a fair number of crash reports with this signature (198 in the last week) though the most recent version reported is 10.0.4 ESR. The data suggest that this is fixed but we have a enterprise users who are not upgraded.
You need to log in before you can comment on or make changes to this bug.