Closed
Bug 580033
Opened 14 years ago
Closed 14 years ago
JS_Save/RestoreFrameChain should update cx->compartment
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
blocking2.0 | --- | beta7+ |
People
(Reporter: jorendorff, Assigned: jorendorff)
References
Details
(Whiteboard: [compartments][fixed-in-tracemonkey])
Attachments
(1 file, 4 obsolete files)
4.91 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
JS_Save/RestoreFrameChain(cx), and more generally any API that affects JS_GetScopeChain(cx), should also update cx->compartment consistently. Spun off from bug 563106 comment 14.
Assignee | ||
Comment 1•14 years ago
|
||
Assignee: general → jorendorff
Attachment #458424 -
Flags: review?(gal)
Updated•14 years ago
|
Attachment #458424 -
Flags: review?(gal) → review+
Assignee | ||
Comment 2•14 years ago
|
||
I tried landing this, but it bounced. In debug mode we hit an assertion in jswrapper.cpp. Debugging.
Assignee | ||
Updated•14 years ago
|
Blocks: compartments
Updated•14 years ago
|
blocking2.0: --- → beta5+
Updated•14 years ago
|
Whiteboard: [compartments]
Assignee | ||
Comment 3•14 years ago
|
||
One problem with this was if we navigate between SaveFrameChain and RestoreFrameChain, the Restore might not restore the previous compartment but switch to a new one. I just re-pushed this patch to the try server to see if that was the issue comment 2 refers to or not...
Assignee | ||
Comment 4•14 years ago
|
||
Blake and I think maybe we don't need this patch if all the DOM code always does stuff in the right compartment (bug 586450). I still think we're better off with it, because "you are always in the same compartment as JS_GetScopeChain(cx)" is a nice invariant, but it doesn't need to block anything. (The Try Server run is green so far, but not done yet.)
Comment 5•14 years ago
|
||
We definitely need something like this patch. In bug 584260, I added an extra call to JS_SetGlobalObject(cx, outerObject) on each page transition in order to avoid letting cx->compartment point to a compartment that could die. There is a problem remaining with that approach though: which is that if there is JS code already running, we'll still restore the original context when we get back to the main event loop (after all of the JSAutoCrossCompartmentCalls pop off the stack).
Updated•14 years ago
|
blocking2.0: beta5+ → beta6+
Assignee | ||
Comment 6•14 years ago
|
||
Landed a rebased version of this, but it bounced. I wonder which would be easier: making the innerObject hook infallible, or making JSContext::setCurrentRegs fallible.
Assignee | ||
Comment 7•14 years ago
|
||
Not quite as elegant. :-\ This patch makes us possibly 4ms slower on Sunspider.
Attachment #458424 -
Attachment is obsolete: true
Comment 8•14 years ago
|
||
Cachegrind results for patch v2 (on Mac 32-bit): --------------------------------------------------------------- | millions of instructions executed | | total | on-trace (may overestimate) | --------------------------------------------------------------- | 189.334 189.407 (------) | 48.623 48.623 (------) | 3d-cube | 136.569 136.575 (------) | 25.519 25.519 (------) | 3d-morph | 192.151 192.216 (------) | 44.991 44.991 (------) | 3d-raytrace | 122.594 122.729 (0.999x) | 14.797 14.797 (------) | access-binary- | 190.615 190.650 (------) | 93.426 93.426 (------) | access-fannkuc | 116.416 116.437 (------) | 17.695 17.695 (------) | access-nbody | 126.129 126.131 (------) | 28.381 28.381 (------) | access-nsieve | 95.116 95.113 (------) | 3.620 3.620 (------) | bitops-3bit-bi | 124.499 124.498 (------) | 32.911 32.911 (------) | bitops-bits-in | 103.538 103.533 (------) | 12.381 12.381 (------) | bitops-bitwise | 130.910 130.912 (------) | 38.378 38.378 (------) | bitops-nsieve- | 109.405 109.399 (------) | 17.896 17.896 (------) | controlflow-re | 125.320 125.333 (------) | 6.671 6.671 (------) | crypto-md5 | 108.623 108.645 (------) | 7.533 7.533 (------) | crypto-sha1 | 161.373 162.529 (0.993x) | 23.114 23.114 (------) | date-format-to | 165.249 165.338 (0.999x) | 9.734 9.734 (------) | date-format-xp | 130.799 130.799 (------) | 31.465 31.465 (------) | math-cordic | 110.600 110.598 (------) | 6.462 6.461 (------) | math-partial-s | 109.773 109.800 (------) | 14.028 14.027 (------) | math-spectral- | 138.506 138.499 (------) | 36.119 36.118 (------) | regexp-dna | 118.444 118.444 (------) | 10.928 10.928 (------) | string-base64 | 197.050 197.324 (0.999x) | 23.937 23.937 (------) | string-fasta | 212.630 216.788 (0.981x) | 19.623 19.623 (------) | string-tagclou | 259.817 273.745 (0.949x) | 29.550 29.549 (------) | string-unpack- | 133.682 133.686 (------) | 8.841 8.841 (------) | string-validat ------- | 3609.152 3629.139 (0.994x) | 606.634 606.631 (------) | all
Comment 9•14 years ago
|
||
Cachegrind differential profile. Positive numbers mean that more instructions were executed in the function.
Comment 10•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/84b4d4856e1e
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Updated•14 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 11•14 years ago
|
||
Thanks njn. Enormously useful data.
Attachment #475641 -
Attachment is obsolete: true
Attachment #475706 -
Attachment is obsolete: true
Attachment #475915 -
Flags: review?(mrbkap)
Updated•14 years ago
|
Attachment #475915 -
Flags: review?(mrbkap) → review+
Assignee | ||
Comment 12•14 years ago
|
||
Landed again, bounced again. We assert in js::AutoCompartment::leave that if there's a frame left on the stack, the origin (compartment we originally departed from and are returning to) is the same as the compartment of that frame's scopeChain. That assertion trips in the browser. I have it in the debugger. Both the origin and the scopeChain are in different non-default compartments. Hmm.
Updated•14 years ago
|
Whiteboard: [compartments] → [compartments][has reviewed patch]
Assignee | ||
Comment 13•14 years ago
|
||
v3 basically asserted in AutoCompartment::leave that cx->compartment had been correct before entering an AutoCompartment. That was too optimistic (yet). So the new code asserts something rather weaker: if cx->compartment is correct when entering AutoCompartment::enter, it'll be correct again when leaving AutoCompartment::leave. The code to do this looks lame, which I think is the right thing.
Attachment #475915 -
Attachment is obsolete: true
Attachment #477643 -
Flags: review?(mrbkap)
Assignee | ||
Comment 14•14 years ago
|
||
Pushed this to the try server, and it came back green.* *with one known orange
Assignee | ||
Comment 15•14 years ago
|
||
Review ping.
Whiteboard: [compartments][has reviewed patch] → [compartments]
Comment 16•14 years ago
|
||
Is this ready or what?
Comment 17•14 years ago
|
||
This is landed in the queue (with a fix to make JS_RestoreFrameChain properly restore cx->compartment).
Comment 18•14 years ago
|
||
So does that mean that it's ready? There's a patch with an outstanding review request which either needs to be stamped or obsoleted and replaced with whatever is actually being checked in.
Whiteboard: [compartments] → [compartments][needs status update]
Comment 19•14 years ago
|
||
Please note that we have now created a branch for beta 7 work. In addition to landing your fix on mozilla-central default, please also land it on mozilla-central GECKO20b7pre_20101006_RELBRANCH (note: when landing on mozilla-central default, you will be given priority in the landing queue at https://wiki.mozilla.org/LandingQueue )
Updated•14 years ago
|
Attachment #477643 -
Flags: review?(mrbkap) → review+
Assignee | ||
Updated•14 years ago
|
Whiteboard: [compartments][needs status update] → [compartments][fixed-in-tracemonkey]
Comment 20•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/c23e2b6f20f7
Status: REOPENED → RESOLVED
Closed: 14 years ago → 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•