879139 - Use-after-free with SVG animation in SVG image

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jesse Ruderman]

Attachment: testcase (see comment 0)

With:
     user_pref("privacy.sanitize.sanitizeOnShutdown", true);

The testcase causes a crash on shutdown.

==40362== ERROR: AddressSanitizer heap-use-after-free on address 0x000146dcc780 at pc 0x10c3379a1 bp 0x7fff5fbeae50 sp 0x7fff5fbeae48
READ of size 8 at 0x000146dcc780 thread T0
    #0 0x10c33799f in nsNodeUtils::LastRelease [nsNodeUtils.cpp:187]
    #1 0x10b9e6fda in mozilla::dom::FragmentOrElement::Release [FragmentOrElement.cpp:1711]
    #2 0x111e73b11 in nsSVGElement::Release [nsSVGElement.cpp:229]
    #3 0x111c013e1 in mozilla::dom::SVGGraphicsElement::Release [SVGGraphicsElement.cpp:15]
    #4 0x111d52f51 in mozilla::dom::SVGSVGElement::Release [SVGSVGElement.cpp:143]
    #5 0x119d1bfc8 in nsXPCOMCycleCollectionParticipant::UnrootImpl [nsCycleCollectionParticipant.cpp:38]
    #6 0x11a1dfd19 in nsCycleCollector::CollectWhite [nsCycleCollector.cpp:2432]
    #7 0x11a1d107f in nsCycleCollector::FinishCollection [nsCycleCollector.cpp:2883]
    #8 0x11a1e3aec in nsCycleCollector::ShutdownCollect [nsCycleCollector.cpp:2767]
    #9 0x11a1e514e in nsCycleCollector::Shutdown [nsCycleCollector.cpp:2917]
    #10 0x11a1e7e11 in nsCycleCollector_shutdown [nsCycleCollector.cpp:3116]
    #11 0x119d709c7 in mozilla::ShutdownXPCOM [nsXPComInit.cpp:651]
    #12 0x119d6fc58 in NS_ShutdownXPCOM [nsXPComInit.cpp:540]
    #13 0x1068e3a62 in ScopedXPCOMStartup::~ScopedXPCOMStartup [nsAppRunner.cpp:1125]
    #14 0x1068e35a8 in ScopedXPCOMStartup::~ScopedXPCOMStartup [nsAppRunner.cpp:1106]
    #15 0x10690c310 in XREMain::XRE_main [nsAppRunner.cpp:3944]
    #16 0x10690db22 in XRE_main [nsAppRunner.cpp:4121]
    #17 0x1000083ec in do_main [nsBrowserApp.cpp:272]
    #18 0x100004b02 in main [nsBrowserApp.cpp:632]
    #19 0x1000015c3 in start (in firefox-bin) + 51
    #20 0x5 in 0x00000005 (in firefox-bin)

Same symptoms as bug 851416, pretty much.

Comment 1

[Daniel Holbert [:dholbert]]

(In reply to Jesse Ruderman from comment #0)
> With:
>      user_pref("privacy.sanitize.sanitizeOnShutdown", true);
> 
> The testcase causes a crash on shutdown.

I can reproduce this without any pref tweaks by clearing my cache (ctrl+shift+del) before shutdown, or by viewing the testcase in a Private Window (ctrl+shift+p) and then quitting.

(So basically: this crashes at shutdown IFF we've cleared the image cache.)

Comment 2

[Daniel Holbert [:dholbert]]

(In reply to Jesse Ruderman from comment #0)
> Same symptoms as bug 851416, pretty much.

Caused by the same commit, too. On my Linux system, targeted debug builds (w/ my cache-clearing STR from comment 1) indicate that this started with https://hg.mozilla.org/mozilla-central/rev/1cf12e699dc7 for bug 846028.

So: This affects builds going back to Firefox 22, which is currently on Beta (slated to be released June 21 I believe).  We should fix this before 22 goes to release, if at all possible.

Seth, do you have cycles to look at this?  Tentatively assigning to you, but feel free to kick it over to me if you need to.

Comment 3

[Seth Fowler [:seth] [:s2h]]

I will take a look tomorrow.

Comment 4

[Alex Keybl [:akeybl]]

Whether or not we fix this FF22 regression will depend on the fix and the risk associated.

Comment 5

[Seth Fowler [:seth] [:s2h]]

Sorry for the MIME type change but I simply could not convince Firefox to download the file otherwise.

Comment 6

[Seth Fowler [:seth] [:s2h]]

Attachment: Keep VectorImage's rendering and mutation observers in sync.

This patch fixes the crash for me locally. It needs a try run before review, though.

Comment 7

[Seth Fowler [:seth] [:s2h]]

Try job here: https://tbpl.mozilla.org/?tree=Try&rev=5893d3103ff3

Comment 8

[Jesse Ruderman]

> Sorry for the MIME type change but I simply could not convince Firefox to download 
> the file otherwise.

Is something wrong with right-click "Save Link As..." in your Firefox?

Comment 9

[Seth Fowler [:seth] [:s2h]]

(In reply to Jesse Ruderman from comment #8)
> > Sorry for the MIME type change but I simply could not convince Firefox to download 
> > the file otherwise.
> 
> Is something wrong with right-click "Save Link As..." in your Firefox?

I think BugzillaJS is at fault, but yes, for some reason I could not do that for this particular test case.

Comment 10

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 760636 [details] [diff] [review]
Keep VectorImage's rendering and mutation observers in sync.

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

This looks OK on try, so requesting review.

Comment 11

[Daniel Holbert [:dholbert]]

Comment on attachment 760636 [details] [diff] [review]
Keep VectorImage's rendering and mutation observers in sync.

It makes me a little uneasy that we're needing to juggle multiple observer lists here, while our parent class also tries to juggle them.

Maybe we should be calling the parent's StartListening / StopListening methods, and let those do the work for us (even though it may sometimes involve a little extra work, e.g. checking the rendering-observer list when we know we're not in it.  Worth the complexity win if it works, IMHO.)

So: does it work if we just switch to using those methods, and let VectorImage be blissfully ignorant of these various lists?

Comment 12

[Seth Fowler [:seth] [:s2h]]

Attachment: Keep VectorImage's rendering and mutation observers in sync.

Thanks for the review, Daniel. I definitely like that idea. I'm not ultra happy with the API that nsSVGRenderingObserver exposes for its subclasses - in particular, we need to call different functions to add ourselves to each observer list, but only a single function to remove ourselves from both, and it's not safe to call StartListening twice without a StopListening in between. This is the reason why, in the updated version of the patch, I continue to call GetReferencedElement manually in one place. (Though it could be replaced by StopListening followed by ResumeListening.) That said, this updated version of the patch does feel cleaner.

Try job here (reftests only): https://tbpl.mozilla.org/?tree=Try&rev=c28aeacada91

Comment 13

[Seth Fowler [:seth] [:s2h]]

Hmmm.. so that doesn't look so good on try. I'll take a look and see what I can do.

Comment 14

[Daniel Holbert [:dholbert]]

Summarizing 2 things seth & I discussed/realized over IRC:

 (1) This VectorImage (& its SVGDocumentWrapper, etc) seem to be sticking around until shutdown, even after I clear my cache, per comment 1.  That seems odd.

 (2) Since it's sticking around until shutdown, it gets some ties severed by the SVGDocumentWrapper's shutdown-observer, which clears the internal doc's rendering-observer list but *not* its mutation observer list. It technically should be clearing both lists. (or, removing everything in the rendering observer list from the mutation observer list as well) Though seth has some ideas about how we could handle this better, which I need to think through a bit more & haven't yet.

Comment 15

[Daniel Holbert [:dholbert]]

For now, I wonder if we should just back out bug 846028 (and bug 851416, which stacks on top of it) from beta, so that we don't ship this with Firefox 22?

akeybl, how much longer do we have (if any time at all) to land backout-patches that restore us to a known-good state, for beta?

Comment 16

[Seth Fowler [:seth] [:s2h]]

From my perspective backing out is a good idea. If we do go with my plan (don't remove ourselves from the observer lists at all, just use a flag in nsSVGRootRenderingObserver to ignore invalidations that we don't want to act on) then I'd be reverting bug 846028 and bug 851416 anyway.

Comment 17

[Seth Fowler [:seth] [:s2h]]

We've agreed to move to the flag approach. I'll work on this in bug 883416. That bug doesn't mention this issue, and justifies the change as a refactoring to simplify the code. (Which is also true!) Consider that bug an implicit blocker of this bug.

Comment 18

[Daniel Holbert [:dholbert]]

Thanks, Seth!

Comment 19

[Jesse Ruderman]

WFM on mozilla-central??

Comment 20

[Seth Fowler [:seth] [:s2h]]

We'll make the change in bug 883416 regardless. I'm not convinced that we've seen the end of bugs from the code as it currently stands. =) I intend to start work on that bug tomorrow.

Comment 21

[Daniel Holbert [:dholbert]]

(In reply to Jesse Ruderman from comment #19)
> WFM on mozilla-central??

Not WFM -- I can still reproduce this with the ctrl+shift+del STR from Comment 1, with current m-c tip. (revision 8ea92aeab783)

Comment 22

[Seth Fowler [:seth] [:s2h]]

So with the patches up in bug 883416 applied, I can't reproduce this with any of the STR mentioned in this bug. I think bug 883416 takes care of this.

Comment 23

[Daniel Holbert [:dholbert]]

Sounds like this should be FIXED, now that bug 883416 has landed, right?

Comment 24

[Daniel Holbert [:dholbert]]

Yup; I get no shutdown crash w/ comment 1's ctrl+shift+del STR, with:
https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013/06/2013-06-23-mozilla-central-debug/firefox-24.0a1.en-US.debug-linux-x86_64.txt
but I do get a crash in the build from the day before:
https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013/06/2013-06-22-mozilla-central-debug/firefox-24.0a1.en-US.debug-linux-x86_64.txt

(And bug 883416's fix landed on mozilla-central between those builds.)

So this is FIXED by bug 883416, which means it's fixed for Firefox 24 and 25, and we don't need to track it for Firefox 25.

Comment 25

[Daniel Holbert [:dholbert]]

[I'll leave mwobensmith's self-needinfo open, though I suspect he toggled that flag to circle back on the tracking-firefox25 request, which is now no longer needed.]

Comment 26

[Matt Wobensmith [:mwobensmith][:matt:]]

Indeed, Daniel. Thanks. :)

Comment 27

[Daniel Holbert [:dholbert]]

Cool! Canceling your needinfo then.

Comment 28

[Seth Fowler [:seth] [:s2h]]

Thanks for marking this as resolved, Daniel!

Comment 29

[Lukas Blakk [:lsblakk] use ?needinfo]

Marking this status-firefox23 fixed as bug 883416 is marked that way as well.