408539 - Storing XPCContext inside JSContext

Status: RESOLVED FIXED

(Core :: XPConnect, enhancement)

Description

[Igor Bukanov]

Currently xpconnect uses a hash table with explicit synchronization calls to update it to maintain the mapping between XPCContext and JSContext. If instead XPCContext would be stored directly in JSContext, the map and the explicit update code would be unnecessary leading to smaller and faster code.

Comment 1

[Igor Bukanov]

Attachment: v1

A prototype implementation.

Ideally to store XPCContext the code should use context-private JSContext.data accessible via public API. But that is taken by the DOM branch callback to store the closure data. There are 2 possibilities to handle that. 

The first one is to store the branch callback closure in XPCConnect itself. That would mean that DOM must be aware about XPCConnect or at least some class that XPCConnect subclasses that stores the closure. 

The second possibility is to add to JSContext something like branchCallbackClosure and use that in the DOM branch callback, not the context-private.

For me the first solution sounds better, but I am not sure if exposure of DOM to xpconnect implementation details is a good thing to do.

For now the patch just adds JSContext.data2 as a hack pending the resolution of the above issues.

Comment 2

[Igor Bukanov]

Attachment: Updated patch

The patch uses the callback to always create XPCContext instance. Compared with the previous version the instance is created eagerly to avoid the problems with lazy creation. The current code pretty much assumes that js->xpc mapping is infallible (it is enforced with asserts) and the patch finally provides the guarantee.

Another thing is that I moved the JS runtime initialization to XPJSRuntime constructor. It allowed to simplify the code.


 that exists in the current code

Comment 3

[Igor Bukanov]

to mrbkap: do you have time to review the patch?

Comment 4

[Igor Bukanov]

Attachment: v2

The new version fixes the context callback initialization issue in xpcshell.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 342591 [details] [diff] [review]
v2

>+nsXPConnect::GetRuntimeInstance( /*= nsnull*/)

Nit: the comment doesn't mean anything anymore.

This is pure win all around.

Comment 7

[Igor Bukanov]

Attachment: v3

The new version addresses the nit and fixes the compilation problem on Windows - I missed to rename nsXPConnect::GetRuntime into nsXPConnect::GetRuntimeInstance in Windows-only  XPCIDispatchExtension.cpp. The try server is a nice thing :)

Comment 8

[Igor Bukanov]

landed - http://hg.mozilla.org/mozilla-central/rev/09fcb1bab6eb

Comment 9

[timeless]

this broke jsd.

Comment 10

[Igor Bukanov]

(In reply to comment #9)
> this broke jsd.

Could you give more details?

Comment 11

[Igor Bukanov]

Recording the presence of the patch on 1.9.1 branch.