Storing XPCContext inside JSContext

RESOLVED FIXED

Status

()

--
enhancement
RESOLVED FIXED
11 years ago
7 years ago

People

(Reporter: igor, Assigned: igor)

Tracking

({fixed1.9.1})

Trunk
fixed1.9.1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

11 years ago
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.
(Assignee)

Comment 1

11 years ago
Created attachment 293365 [details] [diff] [review]
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.
(Assignee)

Comment 2

10 years ago
Created attachment 342440 [details] [diff] [review]
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
Attachment #293365 - Attachment is obsolete: true
Attachment #342440 - Flags: review?(mrbkap)
(Assignee)

Comment 3

10 years ago
to mrbkap: do you have time to review the patch?
(Assignee)

Comment 4

10 years ago
Created attachment 342591 [details] [diff] [review]
v2

The new version fixes the context callback initialization issue in xpcshell.
Attachment #342591 - Flags: review?(mrbkap)
(Assignee)

Updated

10 years ago
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
(Assignee)

Comment 7

10 years ago
Created attachment 342728 [details] [diff] [review]
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 :)
Attachment #342591 - Attachment is obsolete: true
Attachment #342728 - Flags: superreview+
Attachment #342728 - Flags: review+
(Assignee)

Comment 8

10 years ago
landed - http://hg.mozilla.org/mozilla-central/rev/09fcb1bab6eb
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Comment 9

10 years ago
this broke jsd.

Updated

10 years ago
Blocks: 469795
(Assignee)

Comment 10

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

Could you give more details?
(Assignee)

Updated

10 years ago
Blocks: 431698
(Assignee)

Comment 11

10 years ago
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.