Bug 13701 – REGRESSION (r21431): Reproducible crash resulting from calling adoptNode on a password field

• Component: New Bugs
• Status: RESOLVED FIXED

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.

Created attachment 14523 [details]
Test case (following the instructions will lead to a crash)

[Maciej Stachowiak]

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.

[Brady Eidson]

<rdar://problem/5199362>

[Brady Eidson]

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

[Brady Eidson]

Created attachment 14526 [details]
Handle leaving and entering documents

[Maciej Stachowiak]

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.

[Maciej Stachowiak]

Comment on attachment 14526 [details]
Handle leaving and entering documents

r- for above comments; discussing

[Brady Eidson]

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)

[Brady Eidson]

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 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.

[Brady Eidson]

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  ;)

(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>
:-)

[Brady Eidson]

How many 1000's of layout tests hit it 838 times?  ;)

Discussing on IRC right now, seems there's agreement the fear was unfounded.

(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?

[Brady Eidson]

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 on attachment 14528 [details]
Better still

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

r=me

[Brady Eidson]

Suggestion taken, landed in r21447