Summary: | REGRESSION(r49385): crashes in SVGImage due to static EmptyPluginHalterClient | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dimitri Glazkov (Google) <dglazkov> | ||||||||
Component: | SVG | Assignee: | Jon Honeycutt <jhoneycutt> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, darin, eric, jhoneycutt, mitz, mrowe | ||||||||
Priority: | P1 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | PC | ||||||||||
OS: | OS X 10.5 | ||||||||||
Bug Depends on: | 30119 | ||||||||||
Bug Blocks: | |||||||||||
Attachments: |
|
Description
Dimitri Glazkov (Google)
2009-10-15 13:08:56 PDT
Created attachment 41242 [details]
SVGImage crash fix, v1.
I attempted to write one layout test to trigger the crash, but so far had no luck. Adding mitz since he reviewed the http://trac.webkit.org/changeset/49385. 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.
mitz or whoever was involved in the original patch should really review this though. 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. 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? Yes, passing 0 here is a good idea. Created attachment 41286 [details]
SVGImage crash fix, v2.
Comment on attachment 41286 [details]
SVGImage crash fix, v2.
ugh.
Created attachment 41287 [details]
SVGImage crash fix, v2.
Yes. If you're used to git-send-bugzilla, bugzilla-tool post-commits' behavior is confusing, I agree. :( 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?
Yup. That's why I changed the bug ownership to Jon. This is just a fix for the crash. 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 on attachment 41287 [details]
SVGImage crash fix, v2.
git svn rebase is taking foreeeeeveeeer.
Comment on attachment 41287 [details] SVGImage crash fix, v2. Clearing flags on attachment: 41287 Committed r49685: <http://trac.webkit.org/changeset/49685> All reviewed patches have been landed. Closing bug. |