628758 - Startup compartment assertion failure with firebug

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

Browser asserts on startup, in jsd.

#3  0x00007ffff64f6741 in js::CompartmentChecker::check (this=0x7fffffff80b0, c=0x7fffe39f1000) at /home/dvander/mozilla/tracemonkey/js/src/jscntxtinlines.h:557
#4  0x00007ffff64f67f6 in js::CompartmentChecker::check (this=0x7fffffff80b0, str=0x7fffd81c1a40 "TypeError: this.mEditor is undefined")
    at /home/dvander/mozilla/tracemonkey/js/src/jscntxtinlines.h:570
#5  0x00007ffff64fba19 in js::assertSameCompartment<JSString*> (cx=0x7fffd8b95c00, t1=0x7fffd81c1a40 "TypeError: this.mEditor is undefined")
    at /home/dvander/mozilla/tracemonkey/js/src/jscntxtinlines.h:637
#6  0x00007ffff64ef421 in JS_GetStringCharsZAndLength (cx=0x7fffd8b95c00, str=0x7fffd81c1a40 "TypeError: this.mEditor is undefined", plength=0x7fffffff8148)
    at /home/dvander/mozilla/tracemonkey/js/src/jsapi.cpp:5277
#7  0x00007ffff5d3cf38 in jsdValue::GetStringValue (this=0x7fffd4aeebf0, _rval=...) at /home/dvander/mozilla/tracemonkey/js/jsd/jsd_xpc.cpp:2293

Comment 1

[Andreas Gal :gal]

Steve, can you take a look? And David, do you have a testcase?

Comment 2

[Steve Fink [:sfink] [:s:]]

Can't say I'm surprised. When I saw that strings would be compartment-checked, I braced myself for this.

I haven't been able to reproduce with a simple script that throws a TypeError. Google search for this.mEditor showed BlueGriffon. Do you have STR?

Comment 3

[Luke Wagner [:luke]]

David said it was just on startup with firebug.

Comment 4

[Luke Wagner [:luke]]

Wow, there are lots of places in jsd_xpc.cpp where we call JSD_GetDefaultJSContext and then use a JSAPI function without entering a compartment.  Sometimes we enter requests, sometimes not.  Unless there is some global invariant protecting a lot of this, it seems like this code needs an audit of all calls to JSD_GetDefaultJSContext.

Comment 5

[Steve Fink [:sfink] [:s:]]

Attachment: String compartment mismatch (test case is for another issue, but works for this)

Hm. Maybe I should read the code I've been hacking on sometime. :-) I was sort of assuming that it started out sane. I'll do an audit.

I was able to reproduce this bug with a test case for an unrelated issue. It happens on the initial post-Firebug reload.

Comment 6

[Steve Fink [:sfink] [:s:]]

Attachment: Various compartment fixes for JSD

Here's a patch that (I think) clears up a bunch of the compartment issues in JSD, including the one for this bug.

It copies JSStrings when storing them into jsdValues, because we need to use them at later points in time when the original compartment is unavailable.

It does not fix missing request problems, because I don't understand the request model.

It also seems to cause memory corruption after a while. Or something else in my JSD patch stack does. Luke, any chance you could look this over (or better yet, take it over)? I need to get back to fixing up the threading issues with my patch for bug 626743, and I'm out of my depth here.

Comment 7

[Steve Fink [:sfink] [:s:]]

Attachment: Blindly add compartment asserts everywhere in jsdbgapi.cpp

This adds compartment asserts everywhere in jsdbgapi.cpp. I suspect it adds a few too many, in fact; I don't know how to avoid hitting some of these (especially some that are hit even without JSD involvement.) I'm not sure of the exact rules for compartments; maybe some of these are safe accesses?

Comment 8

[Steve Fink [:sfink] [:s:]]

Attachment: Remove a bunch of compartment checks

This can be applied after the previous patch. It removes the checks from a bunch of places that I wasn't sure were valid. It's what I've actually been running with.

Comment 9

[Luke Wagner [:luke]]

Comment on attachment 507598 [details] [diff] [review]
Various compartment fixes for JSD

Looks good to me.  jsd_val.c: nothing you can do without switching it to C++ and the RAII wrapper, but blech.

Comment 10

[Steve Fink [:sfink] [:s:]]

Attachment: Various compartment fixes for JSD

That last patch wasn't so good. I thought JS_NewUCString copied the chars, not borrowed them. This changes it to JS_NewUCStringCopyN to fix memory corruption. (The default seems backwards to me, but I don't propose that the API should change.)

I think I may have added some more fixes, too, but I'll have to look at the interdiff to remind myself.

I'm obsoleting the other two patches too because I've moved them over to bug 630471.

Comment 11

[Luke Wagner [:luke]]

Comment on attachment 508689 [details] [diff] [review]
Various compartment fixes for JSD

>+    call = JS_EnterCrossCompartmentCallScript(jsdc->dumbContext, jsdscript->script);
>+    if ( !call ) {
>+        JSD_UNLOCK();
>+        return JS_FALSE;
>+    }

I think this needs the 'free(jsdhook)' like the one below.

> jsd_GetValueFunctionId(JSDContext* jsdc, JSDValue* jsdval)
>@@ -287,28 +296,43 @@ jsd_NewValue(JSDContext* jsdc, jsval val

>         call = JS_EnterCrossCompartmentCall(jsdc->dumbContext, jsdc->glob);
>         if(!call) {
>             JS_EndRequest(jsdc->dumbContext);
>             free(jsdval);
>             return NULL;
>         }
>+        if(JSVAL_IS_STRING(val)) {
>+            chars = JS_GetStringCharsAndLength(jsdc->dumbContext, JSVAL_TO_STRING(val), &length);
>+            ok = !!chars;
>+        }
>+        if (ok)
>+            ok = JS_AddNamedValueRoot(jsdc->dumbContext, &jsdval->val, "JSDValue");
>+        JS_LeaveCrossCompartmentCall(call);
> 
>-        ok = JS_AddNamedValueRoot(jsdc->dumbContext, &jsdval->val, "JSDValue");
>-        JS_LeaveCrossCompartmentCall(call);
>+        if (ok && JSVAL_IS_STRING(val)) {
>+            JSString *strCopy = JS_NewUCStringCopyN(jsdc->dumbContext, chars, length);
>+            if (!strCopy)
>+                ok = JS_FALSE;
>+            else
>+                val = STRING_TO_JSVAL(strCopy);
>+        }
>+

IIUC, objects also need attention here so perhaps we can just use JS_WrapValue.  Now that I think about it, instead of these places where we do the "read the chars, switch compartment, create a string from the chars" trick, we should just use JS_WrapValue(cx, STRING_TO_JSVAL(str)).  Perhaps you can switch to this?  Sorry for not realizing this earlier.

Comment 12

[Steve Fink [:sfink] [:s:]]

Attachment: Various compartment fixes for JSD

I know I've seen JS_WrapValue. Just forgot, I guess.

Patch updated:
 - better match the surrounding code's funky formatting style
 - simplify logic with JS_WrapValue
 - catch a few more cases

I wonder if a JS_WrapString would be useful, too. (Or would that be spelled JS_CopyString?)

Comment 13

[Steve Fink [:sfink] [:s:]]

Attachment: Various compartment fixes for JSD

Doh! Grabbed patch from wrong directory. Should go to sleep before I break something.

Comment 14

[Luke Wagner [:luke]]

Comment on attachment 509061 [details] [diff] [review]
Various compartment fixes for JSD

Cool, thanks!

Comment 15

[Steve Fink [:sfink] [:s:]]

Attachment: Various compartment fixes for JSD

Sorry, Luke, one more. The compartment entry for jsd_NewValue was bogus. I should have realized that when I typed in

  call = JS_EnterCrossCompartmentCall(jsdc->dumbContext, jsdc->glob);

which is guaranteed to be useless -- I'm asking the JSD context to enter its own compartment.

I removed that cross-compartment call entirely and added a comment to describe the policy being followed.

This patch passes the try server. (Then again, so did the previous one. But this one makes it much farther through the Firebug test suite too.)

Comment 17

[Steve Fink [:sfink] [:s:]]

Comment on attachment 509519 [details] [diff] [review]
Various compartment fixes for JSD

Removing that compartment enter was based on a misconception of mine, so that latest patch is probably wrong.

However, I'm pretty sure the previous one was wrong too. It was entering the wrong compartment in some cases.

I think I know how to fix it, but I'm cancelling the review request for now.

Comment 18

[timeless]

sfink: i know that the jsd whitespace style is foreign, but please do try to honor it until you replace it (i'd love to replace it, i've had patches to replace it, but that requires the tree not to be perpetually frozen and to have reviewers available) in a whitespace only bug.

Comment 19

[Steve Fink [:sfink] [:s:]]

Attachment: Various compartment fixes for JSD

I'm going to stop sitting on this one and just submit it. I was delaying because I'm getting all kinds of random memory corruption and crashes when running the Firebug test suite, but that happens independent of this patch and JSD is definitely more correct with it than without.

Change in this version is to enter the right compartment depending on the type of JSD value: the JSD compartment for strings, since we now copy those into the JSD compartment upon creation, and the foreign (debugged) compartment for anything else. Everything else is the same.

timeless: I don't know what whitespace differences you're referring to. I have tried hard to match the style of surrounding code, even when it differs between jsd_*.c and jsd_xpc.cpp. Looking at this patch, I don't see anywhere that I failed. (A much earlier version of the patch had several such problems.) Please point out the specific problems you are referring to.

Comment 20

[Brendan Eich [:brendan]]

I hope timeless is not referring to old rginda crazyland RMS-style space before left paren of actual and formal parameter list style...

  foo (bar, baz);

  void foo (int bar, double baz) { ... }

That style should be retired. Incrementally in coherent hunks if possible. My two cents, anyway!

/be

Comment 21

[timeless]

> +    if ( ! call )
> +        return 0;

i'm mostly pointing to this condition which is definitely not in the same "style" as the conditions above / below (i know that the others don't agree either, but...):
> +    if (pc)
> +        line = JS_PCToLineNumber(jsdc->dumbContext, jsdscript->script, (jsbytecode*)pc);

> +    JS_LeaveCrossCompartmentCall(call);
>  
>      if( line < first )

is the bit i'm talking about. i really hate the file "style", but it has a style, and i heard a rumor we were in a freeze of some sort. typically cleanup doesn't belong in freezes.

if you're going to do a file cleanup, i'd rather it be a distinct bug (i'll happily stamp, i'd imagine brendan would too, and he probably has the power to grant an a=).

personally, i use:
 if (...) / switch (...) / for (...) / while (...)
 foo(...)
 (cond != ition)
but this isn't yet the style of jsd and until it is, i don't think it's right to introduce a number of other styles in a bug fix.

Comment 22

[Luke Wagner [:luke]]

Comment on attachment 510458 [details] [diff] [review]
Various compartment fixes for JSD

> JSString*
> jsd_GetValueString(JSDContext* jsdc, JSDValue* jsdval)
...
>     if(!jsdval->string)
>     {
>         if(JSVAL_IS_STRING(jsdval->val))
...
>         else
>         {
>             call = JSVAL_IS_PRIMITIVE(jsdval->val)
>                    ? NULL
>                    : JS_EnterCrossCompartmentCall(jsdc->dumbContext, JSVAL_TO_OBJECT(jsdval->val));
>             if(!call) {
>                 JS_EndRequest(cx);
>-
>                 return NULL;
>             }

I know its pre-existing (well, at least going back to compartments), but doesn't this logic mean that non-object non-string values won't get a string?  That may be the intention, but this looked like a big fat wrapper around ValueToString in which case I would assume we wanted stringified ints and bools.  Assuming I'm right, its seems like this code really just wants to conditionally enter the compartment of an object.

>         ok = JS_AddNamedValueRoot(jsdc->dumbContext, &jsdval->val, "JSDValue");
>-        JS_LeaveCrossCompartmentCall(call);
>+        if(JSVAL_IS_STRING(val)) {
>+            if (!JS_WrapValue(jsdc->dumbContext, &val)) {
>+                ok = JS_FALSE;
>+            }
>+        }

I think you want 'if (ok && JSVAL_IS_STRING(val))'.  My kingdom for RAII!

>+            JSString *jsstr = JS_DecompileScript (cx, JSD_GetJSScript(mCx, mScript),
>+                                                  "ppscript", 4);
>             if (!jsstr)
>                 return nsnull;
>+
>+            if (!(chars = JS_GetStringCharsAndLength(cx, jsstr, &length)))
>+                return nsnull;
>+        }
>+
>+        script = JS_CompileUCScript (cx, obj, chars, length, "x-jsd:ppbuffer?type=script", 1);
>         if (!script)
>             return nsnull;

There seems to be a GC hazard (a la bug 602274): jsstr is otherwise unrooted while chars/length are used in JS_CompileUCScript.  Thank goodness you are in a .cpp; just use a JS::Anchor<JSString *> that is scoped to include the JS_CompileUCScript.

>+    JSAutoEnterCompartment ac;
>+    if (!ac.enter(cx, JS_GetFunctionObject(fun)))
>+        return NS_ERROR_FAILURE;
>     JSAutoRequest ar(cx);

I think you want to switch the order of these to satisfy the CHECK_REQUEST in JS_EnterCrossCompartmentCall.

Comment 23

[Steve Fink [:sfink] [:s:]]

Attachment: Various compartment, request fixes for JSD

Doh! I was missing a JS_LeaveCrossCompartmentCall(). Not sure how I lost it.

I also went through and tweaked the whitespacing. Looks like I was following the jsd_high.c consensus, that is different from the jsd_val.c consensus. It's actually a complete mixture within the directory (*.c):

  198 x "if( condition..."
  123 x "if(condition..."
   83 x "if (condition..."
    2 x "if ( condition..."

which doesn't even include "!call" vs "! call", so I'm not going to spend any more time thinking about it.

Luke: the whitespace changes will trash the interdiff, of course, but the difference between this one and the previous one you r+'d is switching to

  JSVAL_IS_STRING ? JSD compartment : jsval's compartment

and possibly an addition or restoration of a JS_LeaveCrossCompartmentCall.

Comment 24

[Steve Fink [:sfink] [:s:]]

Argh! Sorry, you managed to get your review comments in while I was typing that up. (And it didn't report a mid-air... weird.)

Ok, I will look at the comments now.

Comment 25

[Steve Fink [:sfink] [:s:]]

Attachment: Various compartment, request fixes for JSD

(In reply to comment #22)
> Comment on attachment 510458 [details] [diff] [review]
> Various compartment fixes for JSD
> 
> 
> I know its pre-existing (well, at least going back to compartments), but
> doesn't this logic mean that non-object non-string values won't get a string? 
> That may be the intention, but this looked like a big fat wrapper around
> ValueToString in which case I would assume we wanted stringified ints and
> bools.  Assuming I'm right, its seems like this code really just wants to
> conditionally enter the compartment of an object.

What you say makes sense. I wish to subscribe to your newsletter.

Ok, I rewrote the function from scratch. Now it reuses strings, and calls ValueToString for everything else. The compartment hopping could probably be simplified, but I didn't immediately see an easy way. All new bugs!

> 
> >         ok = JS_AddNamedValueRoot(jsdc->dumbContext, &jsdval->val, "JSDValue");
> >-        JS_LeaveCrossCompartmentCall(call);
> >+        if(JSVAL_IS_STRING(val)) {
> >+            if (!JS_WrapValue(jsdc->dumbContext, &val)) {
> >+                ok = JS_FALSE;
> >+            }
> >+        }
> 
> I think you want 'if (ok && JSVAL_IS_STRING(val))'.  My kingdom for RAII!

Fixed, thanks.

> >+            JSString *jsstr = JS_DecompileScript (cx, JSD_GetJSScript(mCx, mScript),
> >+                                                  "ppscript", 4);
> >             if (!jsstr)
> >                 return nsnull;
> >+
> >+            if (!(chars = JS_GetStringCharsAndLength(cx, jsstr, &length)))
> >+                return nsnull;
> >+        }
> >+
> >+        script = JS_CompileUCScript (cx, obj, chars, length, "x-jsd:ppbuffer?type=script", 1);
> >         if (!script)
> >             return nsnull;
> 
> There seems to be a GC hazard (a la bug 602274): jsstr is otherwise unrooted
> while chars/length are used in JS_CompileUCScript.  Thank goodness you are in a
> .cpp; just use a JS::Anchor<JSString *> that is scoped to include the
> JS_CompileUCScript.

Ouch! You are right, of course. Fixed.

> >+    JSAutoEnterCompartment ac;
> >+    if (!ac.enter(cx, JS_GetFunctionObject(fun)))
> >+        return NS_ERROR_FAILURE;
> >     JSAutoRequest ar(cx);
> 
> I think you want to switch the order of these to satisfy the CHECK_REQUEST in
> JS_EnterCrossCompartmentCall.

I just fixed one of those elsewhere, so you'd think I would've noticed. Fixed.

Thanks!

Comment 26

[Mike Shaver (:shaver emeritus)]

Pulling back to betaN+, to front-load FB issues.  lw: please expedite review.

Comment 27

[Luke Wagner [:luke]]

Comment on attachment 510744 [details] [diff] [review]
Various compartment, request fixes for JSD

That's some nice compartment-fu in jsd_GetValueString :)

r+ with one fix: you also need a JS::Anchor in the then-branch of CreatePPLineMap.

Comment 28

[Steve Fink [:sfink] [:s:]]

http://hg.mozilla.org/tracemonkey/rev/87dc60c12d24

Comment 29

[timeless]

this caused bug 632544, i'm sorry i missed it in all of the changes, part of the hazard of doing too many things in a changeset.

Comment 30

[Chris Leary [:cdleary] (not checking bugmail)]

If landing this is preventing us from merging, what is the dire reason we shouldn't back this out? Hoping that we'll get 632544 done before we're forced to merge due to code freeze seems like the wrong approach.

Comment 31

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/87dc60c12d24