Bug 15707

Summary: Crash when manipulating document from within an iframe onload function
Product: WebKit Reporter: Alexey Proskuryakov <ap>
Component: FramesAssignee: Alexey Proskuryakov <ap>
Status: RESOLVED FIXED    
Severity: Normal CC: eric
Priority: P1 Keywords: InRadar
Version: 523.x (Safari 3)   
Hardware: Mac   
OS: OS X 10.4   
Bug Depends on:    
Bug Blocks: 15690    
Attachments:
Description Flags
test case
none
proposed fix darin: review+

Description Alexey Proskuryakov 2007-10-26 02:14:11 PDT
Several W3C DOM tests would crash for us if not masked by another bug. Reduction forthcoming.
Comment 1 Alexey Proskuryakov 2007-10-26 02:14:42 PDT
Created attachment 16875 [details]
test case
Comment 2 Alexey Proskuryakov 2007-10-26 04:27:18 PDT
Here's what is going on:
1) There are two subframes, each calls parent.open() from its onload handler.
2) As the first subframe loads and open()s its parent, the parent is destroyed, and calls willRemove() on the second frame.
3) The second frame stops loading, dispatches onload and thus calls parent.open() again!

Naturally, Document::open() causes havoc when entered recursively.
Comment 3 David Kilzer (:ddkilzer) 2007-11-15 09:15:30 PST
<rdar://problem/5602310>
Comment 4 Alexey Proskuryakov 2009-02-18 09:53:59 PST
Created attachment 27751 [details]
proposed fix
Comment 5 Alexey Proskuryakov 2009-02-18 10:06:31 PST
*** Bug 18803 has been marked as a duplicate of this bug. ***
Comment 6 Darin Adler 2009-02-22 23:34:50 PST
Comment on attachment 27751 [details]
proposed fix

r=me

Will this have any performance impact? I am not too happy about "protect". I always hate those.

Could we scope the "n" variable better?

Instead of this:

    for (n = m_firstChild; n; n = n->nextSibling())

we could do this:

    for (RefPtr<Node> n = m_firstChild; n; n = n->nextSibling())

Instead of this:

    while ((n = m_firstChild) != 0) {

we could do this:

    while (RefPtr<Node> n = m_firstChild) {

I think this would give slightly smaller/faster code because construction doesn't have to check the old value for 0.
Comment 7 Eric Seidel (no email) 2009-02-22 23:37:19 PST
I could have sworn we filed (and possibly fixed a) dupe of this for Chromium...?  At least this bug sounds very familiar to me.
Comment 8 Alexey Proskuryakov 2009-02-23 01:04:36 PST
Committed <http://trac.webkit.org/changeset/41133>.

> At least this bug sounds very familiar to me.

See comment 5 :-)
Comment 9 Alexey Proskuryakov 2009-02-23 01:11:47 PST
> Committed <http://trac.webkit.org/changeset/41133>.

And <http://trac.webkit.org/changeset/41134>.