Closed
Bug 907508
Opened 11 years ago
Closed 11 years ago
Remove unused reserved slots on globals and simplify invariants
Categories
(Core :: XPConnect, defect)
Core
XPConnect
Tracking
()
RESOLVED
FIXED
mozilla26
People
(Reporter: bholley, Assigned: bholley)
References
Details
Attachments
(4 files)
1.71 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
1.45 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
2.88 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
1.16 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
bz and I just dug into this, and it looks like we've been sloppy over the past year with the slots we use on globals. I'm going to clean this up - patches coming.
Assignee | ||
Comment 1•11 years ago
|
||
In the old world, XPConnect globals had one reserved slot, which was used to store a private pointer to the XPCWrappedNativeScope. Then, in the new DOM bindings landing (bug 740069), we added two slots, one for DOM_GLOBAL_OBJECT_SLOT, and one for DOM_PROTOTYPE_SLOT. Then, in bug 761707, we removed DOM_GLOBAL_OBJECT_SLOT, but the slot count remained at 3. Then, in bug 797821, we stopped storing the XPCWrappedNativeScope in the slot on the global, and dropped the XPConnect global count from 3 to 2. Given the above, we can safely drop it to 1, here. It's easy to check that this is correct, because reserved slots for globals have a different offset, which is JSCLASS_GLOBAL_SLOT_COUNT. And according to mxr, the only thing defined in terms of that is DOM_PROTOTYPE_SLOT (which takes the same value). This means that all subsequent slots on the global are unused.
Attachment #793254 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 2•11 years ago
|
||
Continuing the analysis of the previous patch, we actually have two free slots. The first comes from the removal of DOM_GLOBAL_OBJECT_SLOT, as in the previous patch. The second comes from the fact that we mirrored the XPConnect slot for the XPCWrappedNativeScope (so that slot offsets would be the same for workers and non-workers), but didn't drop the slot count in bug 797821.
Attachment #793255 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 3•11 years ago
|
||
Attachment #793257 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 4•11 years ago
|
||
mhordecki needs this in bug 898559.
Attachment #793258 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 5•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=511f5b44ed50
Comment 6•11 years ago
|
||
Comment on attachment 793254 [details] [diff] [review] Part 1 - Drop the number of slots on XPConnect globals from 2 to 1. v1 r=me
Attachment #793254 -
Flags: review?(bzbarsky) → review+
Comment 7•11 years ago
|
||
Comment on attachment 793255 [details] [diff] [review] Part 2 - Drop the slot count for worker globals from 3 to 1. v1 r=me
Attachment #793255 -
Flags: review?(bzbarsky) → review+
Comment 8•11 years ago
|
||
Comment on attachment 793257 [details] [diff] [review] Part 3 - Define the slot count for xpconnect and worker globals in terms of the slots defined in DOMJSClass.h. v1 r=me
Attachment #793257 -
Flags: review?(bzbarsky) → review+
Comment 9•11 years ago
|
||
Comment on attachment 793258 [details] [diff] [review] Part 4 - Provide a mechanism for adding extra slots for XPConnect globals. v1 r=me
Attachment #793258 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 10•11 years ago
|
||
Green try push in comment 5. remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/bdb42e4ad1c8 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/0e81fcb3164b remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/b7951f36e051 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/23fe9ded820d
Comment 11•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/bdb42e4ad1c8 https://hg.mozilla.org/mozilla-central/rev/0e81fcb3164b https://hg.mozilla.org/mozilla-central/rev/b7951f36e051 https://hg.mozilla.org/mozilla-central/rev/23fe9ded820d
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in
before you can comment on or make changes to this bug.
Description
•