831846 - Compartment mismatch with evalcx and watch

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: stack

evalcx('').watch("", /()/)

crashes js debug shell on m-c changeset ce9cdd801a73 without any CLI arguments with a compartment mismatch message:

*** Compartment mismatch 0x101845600 vs. 0x10184d400
Segmentation fault: 11

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   118999:39885ae3a597
user:        Brendan Eich
date:        Tue Jan 15 18:17:50 2013 -0800
summary:     Bug 810525 - unregress DecompileValueGenerator change to handle object literal reference bases (r=jandem).

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

s-s because this involves a compartment mismatch.

Brendan, is bug 810525 a likely regressor?

Comment 2

[Andrew McCreight [:mccr8]]

Bug 830389 is another compartment mismatch (without a testcase) involving js_ValueToSource (though likely unrelated).

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Andrew McCreight [:mccr8] from comment #2)
> Bug 830389 is another compartment mismatch (without a testcase) involving
> js_ValueToSource (though likely unrelated).

That seems unlikely to be related. Scoobidiver in bug 821733 comment 11 says that crashes "have almost completely stopped since 21.0a1/20130111".

autoBisect points to changeset 39885ae3a597 which landed yesterday around noon.

Although I won't exactly discount a latent bug appearing occasionally.

Comment 4

[Andrew McCreight [:mccr8]]

Right, crashes with that signature were mostly due to a different print preview issue which has been fixed, but the bug I linked to also involved a compartment mismatch with js_ValueToSource. I've only seen that once, though, so it isn't a common issue.

Comment 5

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Assuming sec-critical pending further analysis, feel free to change if this is not the case.

Comment 6

[:Benjamin Peterson]

Attachment: enter some compartments

We properly enter sandbox's compartment through the CCW over Object.wrap. We then try to generate an error about the regular expression not being callable in the expression decompiler. The expression decompiler starts playing around with objects in the outer global script creating the compartment violation.

This patch makes the expression decompiler be in the compartment of the script's global.

Comment 7

[Brendan Eich [:brendan]]

No more info to add, comment 6 diagnoses. Hoping this can get fast r+ from a compartment expert (cc'ed gal since he is around).

/be

Comment 8

[Andreas Gal :gal]

Comment on attachment 703566 [details] [diff] [review]
enter some compartments

Can you please check that there is no other frame walking path that might have the same problem?

Comment 9

[:Benjamin Peterson]

Attachment: v2

I think I got all the paths. Anyway, here's a patch with an assertion that will go off next time.

Comment 10

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Assigning to benjamin since this has a patch from him awaiting review from gal.

Comment 11

[Christian Holler (:decoder)]

JSBugMon: Cannot process bug: Unknown exception (check manually)

Comment 12

[Brendan Eich [:brendan]]

I invited gal to do an r+. One peer should be enough. Is there a reason for two? Bill seems busy enough for real, also in that he didn't get to the original r? yet.

/be

Comment 13

[:Benjamin Peterson]

gal only f+ last time. I asked him by email if he wanted to finish, but he should I should switch defer to someone else.

Comment 14

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 704358 [details] [diff] [review]
v2

Sorry, this got taken off my list when Andreas added the f+. It looks fine.

It does make sense to audit all the FrameIter uses for compartment mismatches, but we should land this patch now.

Comment 15

[:Benjamin Peterson]

https://hg.mozilla.org/integration/mozilla-inbound/rev/10a03f096499

Comment 16

[Andrew McCreight [:mccr8]]

(In reply to Bill McCloskey (:billm) from comment #14)
> It does make sense to audit all the FrameIter uses for compartment
> mismatches, but we should land this patch now.

File bug 835170 for that.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/10a03f096499

Comment 18

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 19

[:Benjamin Peterson]

Test added:

https://hg.mozilla.org/integration/mozilla-inbound/rev/e589bf878934

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e589bf878934