Summary: | REGRESSION: Browser crash on clicking back button while at link specified above (inspector: ObjC wrapper outlives JS wrapper) | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ross McDonald <ross.mcdonald> | ||||||||||||
Component: | WebCore JavaScript | Assignee: | Darin Adler <darin> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Major | CC: | darin, ggaren, mitz, mjs, sam | ||||||||||||
Priority: | P1 | Keywords: | HasReduction, InRadar, Regression | ||||||||||||
Version: | 523.x (Safari 3) | ||||||||||||||
Hardware: | Mac | ||||||||||||||
OS: | OS X 10.4 | ||||||||||||||
URL: | http://www.csszengarden.com/zengarden-sample.css | ||||||||||||||
Attachments: |
|
Description
Ross McDonald
2007-04-01 02:22:53 PDT
I had the Web Inspector window open at the time, and can recreate this successfully.... Reproducible by doing some b/f navigation while the inspector is open. Using gdb I found out that the crash happens in inspector.js:469. At the time of the crash, focusedNode, which was a JSHTMLDocument has already been deleted (during earlier GC), despite the fact that the Obj-C wrapper (_private->focusedNode) is still alive. The Document's JS wrapper gets unprotected when the bindings root is invalidated when the Frame is cleared. However, the ObjC wrapper sticks around and therefore also the entry in the ObjC wrapper cache mapping the Document to it. Thus the inspector is able to retrieve the ObjC wrapper which now points to a destroyed JS wrapper. Created attachment 13915 [details]
Demo app (source)
To reproduce the bug with this application, build it an run it linked to TOT WebKit. Do the following:
1) Press Return to load the "a" document.
2) Click the Store button to make and retain an Obj-C wrapper for the document.
3) Choose the "about:blank" document from the combo box, to load that into the view. This destroys the JS wrapper for the document.
4) Click the Back button to go back to the "a" document.
5) Click the Use button to pass the document to JavaScript and try to use it. This will trigger the crash.
Created attachment 13916 [details]
Proposed patch
Comment on attachment 13916 [details]
Proposed patch
I don't see any reason to use the rootObject protect/unprotect if we're also going to directly protect/unprotect.
I'm not sure this qualifies as a regression -- it requires use of the inspector so won't affect most users. I'm a little worried about storage leaks. It might be better to guard use of the JS implementation pointer in the WebScriptObject implementation so the object "goes dead" if the root object is gone. I'd like to hear Geoff's comment on that. I agree with Darin. The alternative would enable careless plug-ins to leak the whole window. (Ultimately, I'm on the fence about whether the browser should try to guard against plug-in leaks -- what about direct malloc leaks? or mmap leaks? -- but WebKit has always guarded against this kind of leak, so I think it needs to keep doing so.) (In reply to comment #8) > I'm not sure this qualifies as a regression -- it requires use of the inspector > so won't affect most users. It is a regression since linked against shipping WebKit, the demo app doesn't crash, while linked against TOT it does. > I'm a little worried about storage leaks. It might be better to guard use of > the JS implementation pointer in the WebScriptObject implementation so the > object "goes dead" if the root object is gone. I'd like to hear Geoff's comment > on that. I also realized that the proposed patch didn't solve the problem of calling -callWebScriptMethod... on an object with invalidated root and hitting the ASSERT in RootObject::interpreter() (or crashing). (In reply to comment #10) > calling > -callWebScriptMethod... on an object with invalidated root and hitting the > ASSERT in RootObject::interpreter() (or crashing). Wrong again (this cannot happen because -[WebScriptObject _root] is nil when the root is invalid). Created attachment 13926 [details] Check JS wrapper validity and recreate if needed I don't think it's possible or desirable to "kill" the object as suggested in comment #8 (the way I understood it). This patch just fetches a new JS wrapper for the DOM node if the old one is gone (it is also possible that the root is invalid but the old wrapper is still alive thanks to some other JS object pointing to it or any part of the DOM, in which case I believe the same old wrapper will be refetched and subsequently reprotected by a different, valid root object). Comment on attachment 13926 [details]
Check JS wrapper validity and recreate if needed
This looks like a safer approach to me, but I'd like Geoff and perhaps Maciej to evaluate it too.
In the WebScriptObject API, once a RootObject becomes invalid, any WebScriptObject created with it goes "inert" with respect to JavaScript. This means, for example, that if you call -valueForKey: on such an object, you'll unconditionally get back nil. I don't think that's a great API, but we probably shouldn't change it now. This patch would poke a small hole in that API, allowing you to pass an inert WebScriptObject as an argument to a JavaScript function, even though you couldn't use the WebScriptObject in any other JavaScript context. I see three problems with that: 1. It's inconsistent, and therefore confusing. 2. It doesn't fix the crash in all cases. A WebScriptObject will fail to regenerate its JS counterpart if its document is not in a frame, in which case, it will still vend a stale pointer. 3. Because it resets the WebScriptObject's RootObject, it breaks the (admittedly not very strong) cross-frame scripting security model. I think it's possible to make the object's inert-ness apply when its used as an argument to a function, too. The -_imp method can just return nil if rootObject->isValid() returns false. The tricky part will be finding all the callers of _imp and getting them to respect a nil return value, but I think that's definitely do-able. Comment on attachment 13926 [details]
Check JS wrapper validity and recreate if needed
r- for the issues I mentioned above (sorry, mitz!)
Given Geoff's comments, I know how to fix this, but I can't seem to reproduce the bug. I have a patch sitting on one of my machines that I can attach. I tried using the inspector and then doing back/forward. I immediately hit another crash, so I fixed those first. Once I fixed the crashes I saw, I couldn't reproduce this using the inspector and back/forward. Created attachment 15392 [details]
patch to fix, not this bug, but other problems seen testing inspector with back/forward
I wanted to put this patch up here. I probably should file a new bug report about these other problems I saw and attach the patch to that for review, but for the moment, I'll just do this.
(In reply to comment #17) > Given Geoff's comments, I know how to fix this, but I can't seem to reproduce > the bug. I have a patch sitting on one of my machines that I can attach. Does this bug no longer reproduce with the demo app? (In reply to comment #18) > patch to fix, not this bug, but other problems seen testing inspector with > back/forward Perhaps your patch belongs in bug 14337. (In reply to comment #19) > Does this bug no longer reproduce with the demo app? Oh, I never tried the demo app! <http://trac.webkit.org/projects/webkit/changeset/24493> has made the crash a little harder to reproduce with the demo app (by coalescing two garbage collections). To reproduce the bug with r24493 or later, build the demo app an run it linked to TOT WebKit. Do the following: 1) Press Return to load the "a" document. 2) Click the Store button to make and retain an Obj-C wrapper for the document. 3) Choose the "about:blank" document from the combo box, to load that into the view. This destroys the JS wrapper for the document. 4) Enter "data:text/html,b" in the combo box and press Return to load a "b" document. 5) Click the Back button to go back to about:blank. 6) Click the Back button to go back to the "a" document. 7) Click the Use button to pass the document to JavaScript and try to use it. This will trigger the crash. Created attachment 15630 [details]
check root object for validity whenever using the ObjC wrapper
Comment on attachment 15630 [details]
check root object for validity whenever using the ObjC wrapper
Kevin Decker reviewed this.
Committed revision 24524. |