Bug 27634 – Give toJS a JSDOMGlobalObject parameter

• Component: DOM
• Status: RESOLVED CONFIGURATION CHANGED

[Eric Seidel (no email)]

Give toJS a JSDOMGlobalObject parameter

This completes the plumbing, and fixes almost all of our "wrong prototype" issues covered by bug 27088.

[Eric Seidel (no email)]

Created attachment 33398 [details]
Compiles, but crashes fast/dom/gc-6.html.


---
 74 files changed, 747 insertions(+), 581 deletions(-)

[Eric Seidel (no email)]

This change is crazy-huge.  I need to find a smart way to break it down into smaller chunks.

[Eric Seidel (no email)]

I wonder if we ever forcefully tear-down global objects.  That could explain the 0xbbadbeef I was seeing hit in some lookup table.

[Eric Seidel (no email)]

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?

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

I found the bug. :(

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

At least I think so.  Will fix tomrorow!

[Eric Seidel (no email)]

Fixing the various DOMObject::mark() calls to DOMObjectWithGlobalPointer::mark() calls did not fix the crash. :(  Investigating.

[Eric Seidel (no email)]

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)
        {
            ASSERT(globalObject->scriptExecutionContext());
        }

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

[Eric Seidel (no email)]

Created attachment 33407 [details]
Fully working patch


---
 78 files changed, 764 insertions(+), 593 deletions(-)

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

Comment on attachment 33407 [details]
Fully working patch

Hum.. Nevermind.  I havne' tadded ChangeLogs, so it's not really ready. :(

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

Created attachment 33462 [details]
Remaining changes, mostly needs a few tests


---
 29 files changed, 105 insertions(+), 99 deletions(-)

[Nikolas Zimmermann]

Any progress here Eric?

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

I haven't looked at this bug in a long time, no.

[Zan Dobersek]

The JSDOMGlobalObject parameter seems to now be present in every toJS declaration and implementation. Can this be closed?

[Ahmad Saleem]

[e-mail] - All dependent bugs are closed? Is this patch needed still and have to rebase? or Webkit is now different path and this is WONTFIX. Appreciate if you can confirm. Thanks!

[Alexey Proskuryakov]

We don't need this bug holding an obsolete patch, but should check bug 27088 which tracks the observable issue. Also, yes, it does look like all of this has been done over the years.