Closed Bug 408539 Opened 14 years ago Closed 13 years ago

Storing XPCContext inside JSContext

Categories

(Core :: XPConnect, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: igor, Assigned: igor)

References

Details

(Keywords: fixed1.9.1)

Attachments

(1 file, 3 obsolete files)

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.
Attached patch v1 (obsolete) — Splinter Review
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.
Attached patch Updated patch (obsolete) — Splinter Review
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
Attachment #293365 - Attachment is obsolete: true
Attachment #342440 - Flags: review?(mrbkap)
to mrbkap: do you have time to review the patch?
Attached patch v2 (obsolete) — Splinter Review
The new version fixes the context callback initialization issue in xpcshell.
Attachment #342591 - Flags: review?(mrbkap)
Attachment #342440 - Attachment is obsolete: true
Attachment #342440 - Flags: review?(mrbkap)
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.
Attachment #342591 - Flags: superreview+
Attachment #342591 - Flags: review?(mrbkap)
Attachment #342591 - Flags: review+
Duplicate of this bug: 459165
Attached patch v3Splinter Review
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 :)
Attachment #342591 - Attachment is obsolete: true
Attachment #342728 - Flags: superreview+
Attachment #342728 - Flags: review+
landed - http://hg.mozilla.org/mozilla-central/rev/09fcb1bab6eb
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
this broke jsd.
Blocks: 469795
(In reply to comment #9)
> this broke jsd.

Could you give more details?
Blocks: 431698
Recording the presence of the patch on 1.9.1 branch.
Keywords: fixed1.9.1
Depends on: 711826
You need to log in before you can comment on or make changes to this bug.