| Summary: | REGRESSION: Crash after closing a tab with Google Maps Street View | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Richard Mlynarik <Mly> | ||||||
| Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Major | CC: | mbritto, mitz, rachael, sroret, troyb | ||||||
| Priority: | P1 | Keywords: | GoogleBug, HasReduction, Regression | ||||||
| Version: | 523.x (Safari 3) | ||||||||
| Hardware: | Mac (PowerPC) | ||||||||
| OS: | OS X 10.4 | ||||||||
| Attachments: |
|
||||||||
|
Description
Richard Mlynarik
2007-06-17 22:30:16 PDT
I could reproduce this on first attempt with the same nightly, and then with the latest one, r23682. 1. Opened two tabs with www.google.com, and one with maps.google.com. 2. Searched for San Francisco in the map. 3. Clicked the Street View button. 3. When promised, zoomed in and opened street view. 4. Clicked around a little. 5. Closed the tab, got the same crash. Created attachment 15367 [details]
proposed fix
Comments about the fix :
This fix is preventing the crash without any visible unexpected effects (on GoogleMaps and on the Layout Tests).
The check for the instance on the constructor is not essential but I think it may be useful to prevent future bugs which can look like this one. If the reviewer don't like it he can remove it.
Comments about the test :
There is no test attached to this patch because we need to use tabs to reproduce it and the DumpRenderTree doesn't handle them. If there is another way to reproduce it I don't know it.
Comment on attachment 15367 [details]
proposed fix
Thanks for the patch. any chance you could add a manual test, that has the instructions on what tabs to open? r- to consider making a manual test but I'd appreove this without if it's too awkward to make one.
While the patch fixes the crash, I think it would be better to figure out a way not to stop the plugin before dispatching the unload event, as the unload handler may want to pull state out of the plugin before going away (or tell the plugin to save state). It is inconsistent that it's allowed to do so when unload results from closing a window but not when it results from closing a background tab. Consider filing a follow-up bug about the root cause after the crasher is fixed. Created attachment 15418 [details]
mitz test case & mbritto fix
As we said on the IRC, I joined mitz test case to my fix in this patch.
With this patch we avoid the crash but we need to keep on working to come with a more efficient solution regarding to mitz's comment.
(In reply to comment #5) > As we said on the IRC, I joined mitz test case to my fix in this patch. > With this patch we avoid the crash but we need to keep on working to come with > a more efficient solution regarding to mitz's comment. Mitz's comment is not asking for a "more efficient" fix. There's no problem here with efficiency. The problem is one of incorrect behavior. The unload handler will get incorrect results when it interacts the plug-in's objects when a background tab is closed. Comment on attachment 15418 [details]
mitz test case & mbritto fix
The comment "more efficient" is misleading. There's no efficiency problem here, but rather inconsistent behavior.
The code should put instance->rootObject() into a local variable -- it's not free and calling it twice is slower than calling it once.
But r=me anyway.
Looks like <http://trac.webkit.org/projects/webkit/changeset/24106> fixed this independently as <rdar://problem/5295734>. I fixed this in a slightly different way; I avoid creating a RuntimeObjectImp at all if the root object is destroyed, as there is no point making one for a destroyed plugin. |