Bug 27634 - Give toJS a JSDOMGlobalObject parameter
Summary: Give toJS a JSDOMGlobalObject parameter
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
Depends on: 27566 27588 27640 27641 27643 27644 27645 27661 27741 27745
Blocks: 27088
  Show dependency treegraph
Reported: 2009-07-23 17:59 PDT by Eric Seidel (no email)
Modified: 2013-01-17 03:15 PST (History)
5 users (show)

See Also:

Compiles, but crashes fast/dom/gc-6.html. (173.00 KB, patch)
2009-07-23 18:01 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Fully working patch (177.69 KB, patch)
2009-07-23 23:04 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Remaining changes, mostly needs a few tests (33.84 KB, patch)
2009-07-24 12:35 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2009-07-23 17:59:53 PDT
Give toJS a JSDOMGlobalObject parameter

This completes the plumbing, and fixes almost all of our "wrong prototype" issues covered by bug 27088.
Comment 1 Eric Seidel (no email) 2009-07-23 18:01:01 PDT
Created attachment 33398 [details]
Compiles, but crashes fast/dom/gc-6.html.

 74 files changed, 747 insertions(+), 581 deletions(-)
Comment 2 Eric Seidel (no email) 2009-07-23 18:01:49 PDT
This change is crazy-huge.  I need to find a smart way to break it down into smaller chunks.
Comment 3 Eric Seidel (no email) 2009-07-23 18:07:28 PDT
I wonder if we ever forcefully tear-down global objects.  That could explain the 0xbbadbeef I was seeing hit in some lookup table.
Comment 4 Eric Seidel (no email) 2009-07-23 18:15:13 PDT
In the crash I'm seeing, this call:

Structure* getCachedDOMStructure(JSDOMGlobalObject* globalObject, const ClassInfo* classInfo)
    JSDOMStructureMap& structures = globalObject->structures();
    return structures.get(classInfo).get(); // CRASHING LINE

structures is junk.

I'm not sure when structures would be junk.

In gc-6.html, the frame is navigated, but the global object should still be alive:

function doit()
    var frame = document.getElementById("frame");
    spanB = frame.contentDocument.getElementById("span-B");
    spanB.customProperty = "B";
    frame.onload = frameLoaded;    
    frame.src = "about:blank";    

frameLoaded() does a GC and then does:

    output.innerHTML += spanB.parentNode + "<BR>";

(which is where we're crashing).

I expect that now that spanB.parentNode would be (correctly) trying to use the prototype chain off of the previous window.  Maybe somehow we've destroyed that previous window?
Comment 5 Eric Seidel (no email) 2009-07-23 19:13:50 PDT
OK.  I've verified that the subframe's global object is getting collected, even though the JSHTMLSpanElement is still alive with a mark() function which i know is marking the frame's global object.
Comment 6 Eric Seidel (no email) 2009-07-23 19:16:50 PDT
I found the bug. :(

JSNode::mark() is calling         DOMObject::mark();  direclty instead of DOMObjectWithGlobalPointer::mark()!

At least I think so.  Will fix tomrorow!
Comment 7 Eric Seidel (no email) 2009-07-23 22:44:07 PDT
Fixing the various DOMObject::mark() calls to DOMObjectWithGlobalPointer::mark() calls did not fix the crash. :(  Investigating.
Comment 8 Eric Seidel (no email) 2009-07-23 22:50:02 PDT
Actually, now I"m just hitting an ASSERT in gc-6.

I"m hitting this ASSERT:
        DOMObjectWithGlobalPointer(PassRefPtr<JSC::Structure> structure, JSDOMGlobalObject* globalObject)
            : DOMObject(structure)
            , m_globalObject(globalObject)

Caused by this FIXME:
Document* DOMWindow::document() const
    // FIXME: This function shouldn't need a frame to work.
    if (!m_frame)
        return 0;

I'll comment out the ASSERT for now.  DOMWindow should always be able to reach the Document!  Just like the Document should always be able to reach the JSDOMWindow.  :(
Comment 9 Eric Seidel (no email) 2009-07-23 23:04:32 PDT
Created attachment 33407 [details]
Fully working patch

 78 files changed, 764 insertions(+), 593 deletions(-)
Comment 10 Eric Seidel (no email) 2009-07-23 23:33:31 PDT
Comment on attachment 33407 [details]
Fully working patch

This patch is ready for review.  I just figure it's large enough no one will dare.  Feel welcome to make comments or r+/r- however :)  I will attempt to split it up tomorrow.
Comment 11 Eric Seidel (no email) 2009-07-23 23:34:05 PDT
Comment on attachment 33407 [details]
Fully working patch

Hum.. Nevermind.  I havne' tadded ChangeLogs, so it's not really ready. :(
Comment 12 Eric Seidel (no email) 2009-07-24 03:39:50 PDT
Comment on attachment 33407 [details]
Fully working patch

Most of this patch has been taken care of by sub-bugs.  I need to write a bunch more tests and upload a new copy with the remaining bits tomorrow.
Comment 13 Eric Seidel (no email) 2009-07-24 12:35:08 PDT
Created attachment 33462 [details]
Remaining changes, mostly needs a few tests

 29 files changed, 105 insertions(+), 99 deletions(-)
Comment 14 Nikolas Zimmermann 2009-12-21 13:02:32 PST
Any progress here Eric?
Comment 15 Eric Seidel (no email) 2009-12-23 13:11:11 PST
This patch is correct, it just needs tests.  The testing will take a little effort because it has to be x-frame, making very specific calls and in some cases may need to be careful that the calls aren't already cached.
Comment 16 Eric Seidel (no email) 2009-12-29 09:40:09 PST
I haven't looked at this bug in a long time, no.
Comment 17 Zan Dobersek 2013-01-17 03:15:23 PST
The JSDOMGlobalObject parameter seems to now be present in every toJS declaration and implementation. Can this be closed?