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

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert] (PTO until 2/24)]

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

Comment 1

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Mozilla/5.0 (X11; Linux x86_64; rv:2.0b4pre) Gecko/20100815 Minefield/4.0b4pre

Comment 2

[Daniel Holbert [:dholbert] (PTO until 2/24)]

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.

Comment 3

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Attachment: stylesheet for testcase 1

Comment 4

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Attachment: reduced testcase 1 [crashes browser on tab-close]

Comment 5

[Daniel Holbert [:dholbert] (PTO until 2/24)]

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.

Comment 6

[Daniel Holbert [:dholbert] (PTO until 2/24)]

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.

Comment 7

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Attachment: fix v1

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

Comment 8

[Daniel Holbert [:dholbert] (PTO until 2/24)]

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

Comment 9

[Daniel Holbert [:dholbert] (PTO until 2/24)]

http://hg.mozilla.org/mozilla-central/rev/0074c4f2d561