RESOLVED CONFIGURATION CHANGED 27634
Give toJS a JSDOMGlobalObject parameter
https://bugs.webkit.org/show_bug.cgi?id=27634
Summary Give toJS a JSDOMGlobalObject parameter
Eric Seidel (no email)
Reported 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.
Attachments
Compiles, but crashes fast/dom/gc-6.html. (173.00 KB, patch)
2009-07-23 18:01 PDT, Eric Seidel (no email)
no flags
Fully working patch (177.69 KB, patch)
2009-07-23 23:04 PDT, Eric Seidel (no email)
no flags
Remaining changes, mostly needs a few tests (33.84 KB, patch)
2009-07-24 12:35 PDT, Eric Seidel (no email)
no flags
Eric Seidel (no email)
Comment 1 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(-)
Eric Seidel (no email)
Comment 2 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.
Eric Seidel (no email)
Comment 3 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.
Eric Seidel (no email)
Comment 4 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?
Eric Seidel (no email)
Comment 5 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.
Eric Seidel (no email)
Comment 6 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!
Eric Seidel (no email)
Comment 7 2009-07-23 22:44:07 PDT
Fixing the various DOMObject::mark() calls to DOMObjectWithGlobalPointer::mark() calls did not fix the crash. :( Investigating.
Eric Seidel (no email)
Comment 8 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) { 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)
Comment 9 2009-07-23 23:04:32 PDT
Created attachment 33407 [details] Fully working patch --- 78 files changed, 764 insertions(+), 593 deletions(-)
Eric Seidel (no email)
Comment 10 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.
Eric Seidel (no email)
Comment 11 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. :(
Eric Seidel (no email)
Comment 12 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.
Eric Seidel (no email)
Comment 13 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(-)
Nikolas Zimmermann
Comment 14 2009-12-21 13:02:32 PST
Any progress here Eric?
Eric Seidel (no email)
Comment 15 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.
Eric Seidel (no email)
Comment 16 2009-12-29 09:40:09 PST
I haven't looked at this bug in a long time, no.
Zan Dobersek
Comment 17 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?
Ahmad Saleem
Comment 18 2022-08-20 00:43:23 PDT
rniwa@webkit.org - 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
Comment 19 2022-08-20 09:55:33 PDT
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.
Note You need to log in before you can comment on or make changes to this bug.