JS_Save/RestoreFrameChain should update cx->compartment

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

Other Branch
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 beta7+)

Details

(Whiteboard: [compartments][fixed-in-tracemonkey])

Attachments

(1 attachment, 4 obsolete attachments)

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.
Posted patch v1 (obsolete) — Splinter Review
Assignee: general → jorendorff
Attachment #458424 - Flags: review?(gal)
Attachment #458424 - Flags: review?(gal) → review+
I tried landing this, but it bounced. In debug mode we hit an assertion in jswrapper.cpp. Debugging.
Blocks: compartments
blocking2.0: --- → beta5+
Whiteboard: [compartments]
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...
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.)
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).
blocking2.0: beta5+ → beta6+
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.
Posted patch v2 (obsolete) — Splinter Review
Not quite as elegant. :-\

This patch makes us possibly 4ms slower on Sunspider.
Attachment #458424 - Attachment is obsolete: true
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
Posted file differential profile (obsolete) —
Cachegrind differential profile.  Positive numbers mean that more instructions were executed in the function.
http://hg.mozilla.org/mozilla-central/rev/84b4d4856e1e
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Posted patch v3 (obsolete) — Splinter Review
Thanks njn. Enormously useful data.
Attachment #475641 - Attachment is obsolete: true
Attachment #475706 - Attachment is obsolete: true
Attachment #475915 - Flags: review?(mrbkap)
Attachment #475915 - Flags: review?(mrbkap) → review+
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.
Whiteboard: [compartments] → [compartments][has reviewed patch]
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)
Pushed this to the try server, and it came back green.*

*with one known orange
Review ping.
Whiteboard: [compartments][has reviewed patch] → [compartments]
Is this ready or what?
This is landed in the queue (with a fix to make JS_RestoreFrameChain properly restore cx->compartment).
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]
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 )
Attachment #477643 - Flags: review?(mrbkap) → review+
Whiteboard: [compartments][needs status update] → [compartments][fixed-in-tracemonkey]
http://hg.mozilla.org/mozilla-central/rev/c23e2b6f20f7
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.