Crash dereferencing null [@ ExternalResourceHider ] when closing tab of SVG with external stylesheet

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

({crash, crashreportid, regression})

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 final+)

Details

(crash signature, )

Attachments

(3 attachments)

STR:
 1. Open two tabs.
 2. In one tab, load URL
    ( http://www.mikehemesath.com/svg_markers/demo/marker.svg )
 3. Close that tab.

ACTUAL RESULTS:
Crash [@ ExternalResourceHider ]

The crash seems 100% reproducible, and the crash-stack seems to stay consistent.
bp-92330b78-5623-41df-9130-8da4f2100815
bp-b45e7888-c12e-422b-9af2-238922100815
bp-94926f48-7510-4cda-9881-b7a652100815
Mozilla/5.0 (X11; Linux x86_64; rv:2.0b4pre) Gecko/20100815 Minefield/4.0b4pre
This is a very recent regression.  Requesting blocking.

WORKS:
Mozilla/5.0 (X11; Linux x86_64; rv:2.0b4pre) Gecko/20100811 Minefield/4.0b4pre
Built from http://hg.mozilla.org/mozilla-central/rev/ba956b17d834

BROKEN:
Mozilla/5.0 (X11; Linux x86_64; rv:2.0b4pre) Gecko/20100812 Minefield/4.0b4pre
Built from http://hg.mozilla.org/mozilla-central/rev/cdfff833edf9

Regression pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ba956b17d834&tochange=cdfff833edf9

Not immediately sure what from that log looks most guilty, though.
blocking2.0: --- → ?
Keywords: regression
Attachment #466169 - Attachment description: reduced testcase 1 → reduced testcase 1 [crashes browser on tab-close]
With targeted builds, I narrowed the regression range to:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=84f27c157997&tochange=d03022d86ce6

From that range, bug 577607 looks most suspicious -- in particular, its first part, which is about external-resource teardown.
So we're crashing in this function, on the attempted call to Hide(), because aData->mViewer is null. (in fact, all of the pointers in aData are null)
824 static PLDHashOperator
825 ExternalResourceHider(nsIURI* aKey,
826                       nsExternalResourceMap::ExternalResource* aData,
827                       void* aClosure)
828 {
829   aData->mViewer->Hide();
830   return PL_DHASH_NEXT;
831 }

and that chunk above was indeed added in bug 577607's first patch.

FWIW, the ExternalResource aData's pointers are null because it gets set up via this call:
752   if (NS_FAILED(load->StartLoad(clone, aRequestingNode))) {
753     // Make sure we don't thrash things by trying this load again, since
754     // chances are it failed for good reasons (security check, etc).
755     AddExternalResource(clone, nsnull, nsnull, aDisplayDocument);

Those "nsnull" pointers there end up being used to initialize aData's member variables.

From looking at AddExternalResource, it looks like it's perfectly acceptable to have a null mViewer pointer on an ExternalResource object.  So I think we just need to null-check aData->mViewer before calling Hide() on it.
Blocks: 577607
Posted patch fix v1Splinter Review
(In reply to comment #6)
> So I think we just
> need to null-check aData->mViewer before calling Hide() on it.

Here's a patch to do that.  I haven't tested this fix yet (compiling it with a fresh build right now), but I'll bet this fixes it.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Comment on attachment 466172 [details] [diff] [review]
fix v1

Confirmed that this trivial patch fixes the crash, on both the URL & attached testcase.

(In reply to comment #6)
> From looking at AddExternalResource, it looks like it's perfectly acceptable to
> have a null mViewer pointer on an ExternalResource object.  So I think we just
> need to null-check aData->mViewer before calling Hide() on it.

(Note also that the ~ExternalResource() destructor null-checks mViewer before making calls to its methods.  So I'm pretty sure this added null-check in ExternalResourceHider is what we want.)
Attachment #466172 - Flags: review?(roc)
http://hg.mozilla.org/mozilla-central/rev/0074c4f2d561
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Crash Signature: [@ ExternalResourceHider ]
You need to log in before you can comment on or make changes to this bug.