Bug 30403 - REGRESSION(r49385): crashes in SVGImage due to static EmptyPluginHalterClient
Summary: REGRESSION(r49385): crashes in SVGImage due to static EmptyPluginHalterClient
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P1 Normal
Assignee: Jon Honeycutt
URL:
Keywords:
Depends on: 30119
Blocks:
  Show dependency treegraph
 
Reported: 2009-10-15 13:08 PDT by Dimitri Glazkov (Google)
Modified: 2009-10-16 10:36 PDT (History)
6 users (show)

See Also:


Attachments
SVGImage crash fix, v1. (2.57 KB, patch)
2009-10-15 13:13 PDT, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff
SVGImage crash fix, v2. (3.03 KB, patch)
2009-10-16 08:57 PDT, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff
SVGImage crash fix, v2. (2.55 KB, patch)
2009-10-16 08:59 PDT, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dimitri Glazkov (Google) 2009-10-15 13:08:56 PDT
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.
Comment 1 Dimitri Glazkov (Google) 2009-10-15 13:13:20 PDT
Created attachment 41242 [details]
SVGImage crash fix, v1.
Comment 2 Dimitri Glazkov (Google) 2009-10-15 13:14:53 PDT
I attempted to write one layout test to trigger the crash, but so far had no luck.
Comment 3 Dimitri Glazkov (Google) 2009-10-15 13:21:26 PDT
Adding mitz since he reviewed the http://trac.webkit.org/changeset/49385.
Comment 4 Eric Seidel (no email) 2009-10-15 20:58:46 PDT
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.
Comment 5 Eric Seidel (no email) 2009-10-15 20:59:02 PDT
mitz or whoever was involved in the original patch should really review this though.
Comment 6 Jon Honeycutt 2009-10-15 21:17:45 PDT
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.
Comment 7 Dimitri Glazkov (Google) 2009-10-15 21:25:27 PDT
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?
Comment 8 Jon Honeycutt 2009-10-15 22:32:39 PDT
Yes, passing 0 here is a good idea.
Comment 9 Dimitri Glazkov (Google) 2009-10-16 08:57:35 PDT
Created attachment 41286 [details]
SVGImage crash fix, v2.
Comment 10 Dimitri Glazkov (Google) 2009-10-16 08:58:21 PDT
Comment on attachment 41286 [details]
SVGImage crash fix, v2.

ugh.
Comment 11 Dimitri Glazkov (Google) 2009-10-16 08:59:10 PDT
Created attachment 41287 [details]
SVGImage crash fix, v2.
Comment 12 Eric Seidel (no email) 2009-10-16 08:59:56 PDT
Yes.  If you're used to git-send-bugzilla, bugzilla-tool post-commits' behavior is confusing, I agree. :(
Comment 13 Eric Seidel (no email) 2009-10-16 09:01:10 PDT
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?
Comment 14 Dimitri Glazkov (Google) 2009-10-16 09:04:59 PDT
Yup. That's why I changed the bug ownership to Jon. This is just a fix for the crash.
Comment 15 Eric Seidel (no email) 2009-10-16 09:06:54 PDT
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.
Comment 16 Dimitri Glazkov (Google) 2009-10-16 09:56:25 PDT
Comment on attachment 41287 [details]
SVGImage crash fix, v2.

git svn rebase is taking foreeeeeveeeer.
Comment 17 WebKit Commit Bot 2009-10-16 10:35:59 PDT
Comment on attachment 41287 [details]
SVGImage crash fix, v2.

Clearing flags on attachment: 41287

Committed r49685: <http://trac.webkit.org/changeset/49685>
Comment 18 WebKit Commit Bot 2009-10-16 10:36:03 PDT
All reviewed patches have been landed.  Closing bug.