Bug 64726 – OpaqueJSClass could always lock the first page that uses the class

• Status: NEW
• Component: JavaScriptCore

[Yong Li]

JSObject* OpaqueJSClass::prototype(ExecState* exec)
{
    /* Class (C++) and prototype (JS) inheritance are parallel, so:
     *     (C++)      |        (JS)
     *   ParentClass  |   ParentClassPrototype
     *       ^        |          ^
     *       |        |          |
     *  DerivedClass  |  DerivedClassPrototype
     */
    
    if (!prototypeClass)
        return 0;

    OpaqueJSClassContextData& jsClassData = contextData(exec);

    if (!jsClassData.cachedPrototype) {
        // Recursive, but should be good enough for our purposes
        jsClassData.cachedPrototype = new (exec) JSCallbackObject<JSObjectWithGlobalObject>(exec, exec->lexicalGlobalObject(), exec->lexicalGlobalObject()->callbackObjectStructure(), prototypeClass, &jsClassData); // set jsClassData as the object's private data, so it can clear our reference on destruction
        if (parentClass) {
            if (JSObject* prototype = parentClass->prototype(exec))
                jsClassData.cachedPrototype->setPrototype(prototype);
        }
    }
    return jsClassData.cachedPrototype.get();
}

    OpaqueJSClassContextData& jsClassData = contextData(exec);

Seems different "exec" should return different jsClassData, however they share one copy because they share one JSGlobalData.

Assume multiple pages share a same JSClassDef. "jsClassData.cachedPrototype" is created in the first page's ExecState, and shared by other pages, because they usually share one JSGlobalData. As long as the "cachedPrototype" is being used by other pages, the first page's global objects could always be locked by "cachedPrototype", which could also hold the entire DOM tree in memory.

Oliver, do you think it is a problem or not?

[Geoffrey Garen]

> the first page's global objects could always be locked by "cachedPrototype"

How? When does this happen?

[Yong Li]

(In reply to comment #1)
> > the first page's global objects could always be locked by "cachedPrototype"
> 
> How? When does this happen?

Assume I define a class A, and create object a1 for page1. Then OpaqueJSClass::prototype create a "cachedPrototype" in page1's context.

Then I create object a2 for page2 using the same JSClassDef for class A. WebKit will use the same OpaqueJSClass, and then the same "cachedPrototype".

Now I close page1, which triggers a GC. However the "cachedPrototype" is still locked by object a2. I'm not very clear about how GC marks the global objects, but I'm seeing the JSDOMWindow of page1 is marked due to this. So it is not GC'ed. (This is based on an old checkout, but I still see the relevant OpaqueJSClass code in ToT.)

Then I close page2, a2 releases the lock on "cachedPrototype", then "cachedPrototype" is also collected, and everything in page1's context is cleaned up finally.

[Yong Li]

(In reply to comment #2)
> (In reply to comment #1)
> > > the first page's global objects could always be locked by "cachedPrototype"
> > 
> > How? When does this happen?
> 
> Assume I define a class A, and create object a1 for page1. Then OpaqueJSClass::prototype create a "cachedPrototype" in page1's context.
> 
> Then I create object a2 for page2 using the same JSClassDef for class A. WebKit will use the same OpaqueJSClass, and then the same "cachedPrototype".
> 
> Now I close page1, which triggers a GC. However the "cachedPrototype" is still locked by object a2. I'm not very clear about how GC marks the global objects, but I'm seeing the JSDOMWindow of page1 is marked due to this. So it is not GC'ed. (This is based on an old checkout, but I still see the relevant OpaqueJSClass code in ToT.)
> 
> Then I close page2, a2 releases the lock on "cachedPrototype", then "cachedPrototype" is also collected, and everything in page1's context is cleaned up finally.

Actually a1/a2 is added to "window", and that is probably the reason why it holds JSDOMWindow

[Ahmad Saleem]

I think this bug is miscategorised as `Java` rather than `JavaScript Core`. Updated.

CCed - Alexey & Yusuke - if you can confirm if it is still applicable or not.