Bug 13701 - REGRESSION (r21431): Reproducible crash resulting from calling adoptNode on a password field
Summary: REGRESSION (r21431): Reproducible crash resulting from calling adoptNode on a...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 523.x (Safari 3)
Hardware: Macintosh OS X 10.4
: P1 Major
Assignee: Brady Eidson
URL:
Keywords: InRadar, Regression
Depends on:
Blocks:
 
Reported: 2007-05-12 15:46 PDT by mitz
Modified: 2007-05-13 08:51 PDT (History)
1 user (show)

See Also:


Attachments
Test case (following the instructions will lead to a crash) (600 bytes, text/html)
2007-05-12 15:47 PDT, mitz
no flags Details
Handle leaving and entering documents (2.38 KB, patch)
2007-05-12 18:29 PDT, Brady Eidson
mjs: review-
Details | Formatted Diff | Diff
Better! (3.70 KB, patch)
2007-05-12 19:11 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Better still (5.54 KB, patch)
2007-05-12 21:56 PDT, Brady Eidson
mitz: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2007-05-12 15:46:52 PDT
The attached test case demonstrates that it is possible to crash the browser after using adoptNode() to move a password field out of a document. Password fields register with their document for the didRestoreFromCache() notification, and unregister when they are destroyed. The problem is that if the password field is adopted by a different document before being destroyed, it will unregister with the wrong document (the new one) and a deleted element will remain registered with the original document. When that document is restored from the back/forward cache, it will send the notification to an invalid pointer and crash.

Other issues involving adoptNode and form elements have been mentioned in bug 12938. While I don't think it's a good idea to override setDocument(), a separate method to be used exclusively by adoptNode() might be appropriate.
Comment 1 mitz 2007-05-12 15:47:54 PDT
Created attachment 14523 [details]
Test case (following the instructions will lead to a crash)
Comment 2 Maciej Stachowiak 2007-05-12 17:05:53 PDT
Probably the registration and destruction should happen at insertedIntoDocument and removedFromDocument time. This would naturally handle document adoption and would ignore password fields that are not in the document, which is probably ok.
Comment 3 Brady Eidson 2007-05-12 17:09:17 PDT
<rdar://problem/5199362>
Comment 4 Brady Eidson 2007-05-12 17:56:23 PDT
Fwiw, Mitz's test case causes havok for non-password elements as well.  Change it to input type=text, for example.  Then run through the test.  You won't crash right after clicking to go back to the cached page, but you will crash when the view is torn down (close the tab, close the browser, etc)

This is because of the HTMLInputElements mis-registration for form state (a problem existant before my patch) -
Thread 0 Crashed:
0   <<00000000>> 	0x00000000 0 + 0
1   com.apple.WebCore        	0x01381b60 WebCore::FrameLoader::saveDocumentState() + 356 (FrameLoader.cpp:3767)
2   com.apple.WebCore        	0x01388bf5 WebCore::FrameLoader::closeURL() + 17 (FrameLoader.cpp:628)
3   com.apple.WebCore        	0x01388c50 WebCore::FrameLoader::detachFromParent() + 44 (FrameLoader.cpp:2967)
4   com.apple.WebKit         	0x0045d9e5 -[WebView(WebPrivate) _close] + 451 (WebView.mm:671)

I hope to fix both these in one fell swoop
Comment 5 Brady Eidson 2007-05-12 18:29:33 PDT
Created attachment 14526 [details]
Handle leaving and entering documents
Comment 6 Maciej Stachowiak 2007-05-12 18:59:12 PDT
As commented on IRC: I see you added insertion/removal to insertedIntoDocument / removedFromDocument, but you left in the old registration in setInputType, which does not check if you are in the document. So it seems to me if you dynamically create an input element that is never in a document and adoptNode it without inserting it into the new doc, you can still have a crash. I suggest adding a test case for that case too. r- for now, discussing how to do this better.
Comment 7 Maciej Stachowiak 2007-05-12 19:02:40 PDT
Comment on attachment 14526 [details]
Handle leaving and entering documents

r- for above comments; discussing
Comment 8 Brady Eidson 2007-05-12 19:11:51 PDT
Created attachment 14527 [details]
Better!

This cleans things up very nicely, possibly plugging even more potential crashers that could've happened without a document (dynamic creation)
Comment 9 Brady Eidson 2007-05-12 21:56:46 PDT
Created attachment 14528 [details]
Better still

This is the approach I discussed with Maciej on #webkit and seems to be the "most correct" to date
Comment 10 mitz 2007-05-12 23:16:30 PDT
Comment on attachment 14528 [details]
Better still

I'm concerned about the performance implications of calling a virtual method from setDocument. I thought that could be restricted to when there is an old owner document.
Comment 11 Brady Eidson 2007-05-12 23:28:39 PDT
Turns out Nodes always start out with a document via their constructor, not via setDocument()

setDocument() is only called when some manipulation of the node is occuring manually.  It is an extremely cool method.  I challenge you to set a breakpoint in it and see if you can hit it with any live web page - I've only been able to hit it using your test case above  ;)
Comment 12 mitz 2007-05-12 23:33:20 PDT
(In reply to comment #11)
> Turns out Nodes always start out with a document via their constructor, not via
> setDocument()
> 
> setDocument() is only called when some manipulation of the node is occuring
> manually.  It is an extremely cool method.  I challenge you to set a breakpoint
> in it and see if you can hit it with any live web page - I've only been able to
> hit it using your test case above  ;)
> 

It's called ~840 times by WebKit layout tests: <http://www.openembedded.org/~zecke/coverage/webkit/mac/__WebCore__dom__Node.cpp.html>
:-)
Comment 13 Brady Eidson 2007-05-12 23:45:51 PDT
How many 1000's of layout tests hit it 838 times?  ;)

Discussing on IRC right now, seems there's agreement the fear was unfounded.
Comment 14 mitz 2007-05-13 00:08:13 PDT
(In reply to comment #13)
> How many 1000's of layout tests hit it 838 times?  ;)
> 
> Discussing on IRC right now, seems there's agreement the fear was unfounded.
> 

YES, YOU ARE RIGHT.

So the question now is why not just make setDocument virtual and override it in HTMLInputElement. I guess the answer is that the "notifications" design is nicer and there's less risk of people screwing up in the override?
Comment 15 Brady Eidson 2007-05-13 00:40:35 PDT
I think the "notifications" design is a lot nicer - the "risk of people screwing up in the override" is not worth it, imho.  The Node base class handles the Document, period.  Its subclasses shouldn't get to handle what does or does not happen as far as changing the document - they should simply be aloud to do their own cleanup when it does change.
Comment 16 mitz 2007-05-13 06:41:46 PDT
Comment on attachment 14528 [details]
Better still

I think it's cleaner and safer to call willMoveToNewOwnerDocument() before updating the wrapper caches.

r=me
Comment 17 Brady Eidson 2007-05-13 08:51:04 PDT
Suggestion taken, landed in r21447