580033 - JS_Save/RestoreFrameChain should update cx->compartment

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

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.

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: v1

Comment 2

[Jason Orendorff [:jorendorff]]

I tried landing this, but it bounced. In debug mode we hit an assertion in jswrapper.cpp. Debugging.

Comment 3

[Jason Orendorff [:jorendorff]]

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

Comment 4

[Jason Orendorff [:jorendorff]]

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

[Blake Kaplan (:mrbkap) (inactive)]

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

Comment 6

[Jason Orendorff [:jorendorff]]

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.

Comment 7

[Jason Orendorff [:jorendorff]]

Attachment: v2

Not quite as elegant. :-\

This patch makes us possibly 4ms slower on Sunspider.

Comment 8

[Nicholas Nethercote [inactive]]

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

[Nicholas Nethercote [inactive]]

Attachment: differential profile

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

Comment 10

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/84b4d4856e1e

Comment 11

[Jason Orendorff [:jorendorff]]

Attachment: v3

Thanks njn. Enormously useful data.

Comment 12

[Jason Orendorff [:jorendorff]]

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.

Comment 13

[Jason Orendorff [:jorendorff]]

Attachment: 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.

Comment 14

[Jason Orendorff [:jorendorff]]

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

*with one known orange

Comment 15

[Jason Orendorff [:jorendorff]]

Review ping.

Comment 16

[Robert Sayre]

Is this ready or what?

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

This is landed in the queue (with a fix to make JS_RestoreFrameChain properly restore cx->compartment).

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

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.

Comment 19

[Mike Beltzner [:beltzner, not reading bugmail]]

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 )

Comment 20

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/c23e2b6f20f7