Bug 30403 – REGRESSION(r49385): crashes in SVGImage due to static EmptyPluginHalterClient

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Dimitri Glazkov (Google)]

After http://trac.webkit.org/changeset/49385, the PluginHalter started owning PluginHalterClient, which now makes it necessary for the EmptyPluginHalterClient instance in SVGImage to be non-static (since it can be deleted once PluginHalter is destroyed).

This was caught as a crash in build.chromium.org build bot, when this sequence of tests is run:

LayoutTests/svg/W3C-SVG-1.1/struct-image-01-t.svg
LayoutTests/svg/W3C-SVG-1.1/struct-image-03-t.svg
LayoutTests/svg/W3C-SVG-1.1/struct-image-04-t.svg
LayoutTests/svg/W3C-SVG-1.1/struct-image-05-b.svg
LayoutTests/svg/W3C-SVG-1.1/struct-image-06-t.svg
LayoutTests/svg/W3C-SVG-1.1/struct-image-07-t.svg
LayoutTests/svg/W3C-SVG-1.1/struct-image-08-t.svg
LayoutTests/svg/W3C-SVG-1.1/struct-image-09-t.svg
LayoutTests/svg/W3C-SVG-1.1/struct-image-10-t.svg
LayoutTests/svg/carto.net/scrollbar.svg
LayoutTests/svg/carto.net/selectionlist.svg

The crash occurs when the garbage collection attempts to discard CachedImage holding SVGImage. SVGImage destructor clears its fake page, which destroys PluginHalter and then PluginHalterClient. Doing so results in the static instance being destroyed.

[Dimitri Glazkov (Google)]

Created attachment 41242 [details]
SVGImage crash fix, v1.

[Dimitri Glazkov (Google)]

I attempted to write one layout test to trigger the crash, but so far had no luck.

[Dimitri Glazkov (Google)]

Adding mitz since he reviewed the http://trac.webkit.org/changeset/49385.

[Eric Seidel (no email)]

Comment on attachment 41242 [details]
SVGImage crash fix, v1.

I would put a comment next to the code, explaining the ownership, and why it's non-static.

Something like:
// Create a new EmptyPluginHalterClient, because FooBar owns the client and will destroy it.

[Eric Seidel (no email)]

mitz or whoever was involved in the original patch should really review this though.

[Jon Honeycutt]

Thanks for investigating this, Dimitri!

As Eric said on IRC, we should make PluginHalter / PluginHalterClient behave like the other controllers stored on Page, i.e., the PluginHalter should notify its client when it is being destroyed, and the client may then choose to clean itself up.

[Dimitri Glazkov (Google)]

Sounds good. To stop the crasheage while this is being refactored, perhaps we could check in mine or even a simpler patch that passes 0 as client?

[Jon Honeycutt]

Yes, passing 0 here is a good idea.

[Dimitri Glazkov (Google)]

Created attachment 41286 [details]
SVGImage crash fix, v2.

[Dimitri Glazkov (Google)]

Comment on attachment 41286 [details]
SVGImage crash fix, v2.

ugh.

[Dimitri Glazkov (Google)]

Created attachment 41287 [details]
SVGImage crash fix, v2.

[Eric Seidel (no email)]

Yes.  If you're used to git-send-bugzilla, bugzilla-tool post-commits' behavior is confusing, I agree. :(

[Eric Seidel (no email)]

Comment on attachment 41287 [details]
SVGImage crash fix, v2.

This looks OK.  We still need a bug to fix the ownership semantics of PluginHalterClient I think.  Also, why do we even have EmptyPluginHalterClient defined if we don't use it here anymore?

[Dimitri Glazkov (Google)]

Yup. That's why I changed the bug ownership to Jon. This is just a fix for the crash.

[Eric Seidel (no email)]

Comment on attachment 41287 [details]
SVGImage crash fix, v2.

OK.  It's just a little strange of a state to leave the tree in. :)  But alright.

[Dimitri Glazkov (Google)]

Comment on attachment 41287 [details]
SVGImage crash fix, v2.

git svn rebase is taking foreeeeeveeeer.

[WebKit Commit Bot]

Comment on attachment 41287 [details]
SVGImage crash fix, v2.

Clearing flags on attachment: 41287

Committed r49685: <http://trac.webkit.org/changeset/49685>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.