JS_Save/RestoreFrameChain should update cx->compartment

RESOLVED FIXED

Status

()

RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

Other Branch
Points:
---

Firefox Tracking Flags

(blocking2.0 beta7+)

Details

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

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

8 years ago
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

8 years ago
Created attachment 458424 [details] [diff] [review]
v1
Assignee: general → jorendorff
Attachment #458424 - Flags: review?(gal)

Updated

8 years ago
Attachment #458424 - Flags: review?(gal) → review+
(Assignee)

Comment 2

8 years ago
I tried landing this, but it bounced. In debug mode we hit an assertion in jswrapper.cpp. Debugging.
(Assignee)

Updated

8 years ago
Blocks: 584237

Updated

8 years ago
blocking2.0: --- → beta5+

Updated

8 years ago
Whiteboard: [compartments]
(Assignee)

Comment 3

8 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

8 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.)
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

8 years ago
blocking2.0: beta5+ → beta6+
(Assignee)

Comment 6

8 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

8 years ago
Created attachment 475641 [details] [diff] [review]
v2

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
Created attachment 475706 [details]
differential profile

Cachegrind differential profile.  Positive numbers mean that more instructions were executed in the function.

Comment 10

8 years ago
http://hg.mozilla.org/mozilla-central/rev/84b4d4856e1e
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Updated

8 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 11

8 years ago
Created attachment 475915 [details] [diff] [review]
v3

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+
(Assignee)

Comment 12

8 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.
Whiteboard: [compartments] → [compartments][has reviewed patch]
(Assignee)

Comment 13

8 years ago
Created attachment 477643 [details] [diff] [review]
v4 - less assertive

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

8 years ago
Pushed this to the try server, and it came back green.*

*with one known orange
(Assignee)

Comment 15

8 years ago
Review ping.
Whiteboard: [compartments][has reviewed patch] → [compartments]

Comment 16

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

Updated

8 years ago
Whiteboard: [compartments][needs status update] → [compartments][fixed-in-tracemonkey]

Comment 20

8 years ago
http://hg.mozilla.org/mozilla-central/rev/c23e2b6f20f7
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.