614714 - Assertion failure: obj->containsSlot(slot) with noscript+youtube

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

You're going to hate me for this...

BUILD: Current tip m-c

STEPS TO REPRODUCE:
1)  Build debug
2)  Install https://secure.informaction.com/download/betas/noscript-2.0.6rc2.xpi
2)  Start the browser
4)  Open the error console (not webconsole, note)
5)  Run this script in the typein field there:

  var _ = Components.utils.getGlobalForObject(top.opener.noscriptOverlay.ns); _.DNS._cache.reset(); _.ChannelReplacement.prototype._callSink = function(sink, oldChan, newChan, flags) {  try { dump('aaa'); oldChan.notificationCallbacks = sink; sink.asyncOnChannelRedirect(oldChan, newChan, flags, this._redirectCallback); } catch(e) { _.ns.log(oldChan.name + "\n" + e); throw e }  };

6)  Load http://www.youtube.com/watch?v=7OHKc5ZRlz8

EXPECTED RESULTS: no fatal asserts

ACTUAL RESULTS: Fatal assert

Additional information:

(gdb) frame 1
#1  0x000000010505525c in js::Interpret () at /Users/bzbarsky/mozilla/css-frameconst/mozilla/js/src/jsinterp.cpp:5315
5315        JS_ASSERT(obj->containsSlot(slot));
(gdb) p slot
$1 = 136

|obj| is a ChromeWindow with slots numbered 0 to 123.

This _might_ be related to bug 605015.

Comment 1

[Robert Sayre]

I can still reproduce on today's tip. But I don't understand why this should block.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Well, in an opt build we're reading from a slot that's outside the slots range of the object, so reading some random part of memory, no?  That seems... bad.  Or am I missing something?

I agree it's less of a worry if this can only be triggered by noscript, but we have no data to indicate that.

I should have made that clearer when nominating, sorry.

I can reproduce this quite easily on Mac with rev 37c63f75ff2f (so sometime last night).  I'll pull to see whether something has changed in the last 15 hours, I guess.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Oh, the op here is GETGLOBAL, fwiw.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Test case

This test triggers the same assert (Assertion failure: obj->containsSlot(slot), at ../jsobj.cpp:5088)

Comment 5

[Jan de Mooij [:jandem]]

Btw this is the same assert as in bug 611653 and bug 613619.

Comment 6

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

I looked into this a little. I think I at least understand the problem, if not the solution.

The script that's typed into the error console is being compiled as COMPILE_N_GO. Its global object seems to be some phony thing which is generated anew each time you type something into the console. The script installs a callback that is called by NoScript when you go to the YouTube page. NoScript calls the function without any special magic, which means that now it's being run with a different global object--the one used by NoScript. When the typed-in script accesses the |_| variable in the catch block, it uses GETGLOBAL. Since that global doesn't exist in the NoScript global object, we get an exception.

It seems like a simple fix would be to change the error console so that it doesn't use COMPILE_N_GO. Is that all we need?

Also, I was unable to reproduce Jan's problem. Do you still see it, Jan?

Comment 7

[Boris Zbarsky [:bzbarsky]]

> Its global object seems to be some phony thing which is generated
> anew each time you type something into the console

Its global object is a bog-standard DOM Window.  The console evaluation code just has an <iframe>, and performs the following steps to evaluate a given string:

1)  Load about:blank in the <iframe>.
2)  Set the src of the <iframe> to "javascript:stuff-to-evaluate".

> NoScript calls the function without any special magic

What does that mean, exactly?  How is this situation different from a function in any web page being called from some other web page at the same domain?

> It seems like a simple fix would be to change the error console so that it
> doesn't use COMPILE_N_GO.

The error console is just doing things that any web page out there can do.  If those things are a problem, we need to disable them globally, not just for the error console...

Comment 8

[Boris Zbarsky [:bzbarsky]]

Oh, in case it matters javascript: evaluation just goes through nsJSContext::EvaluateString (which is what <script> tags in HTML go through as well), and that just calls JS_EvaluateUCScriptForPrincipalsVersion.  This last is what decides to COMPILE_N_GO (which makes sense, since we're just evaluating the string once).

Comment 9

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

Attachment: JS_ClearScope stack trace

I made a mistake when trying to determine the global object. I've now verified that the global object in which we BINDGNAME the |_| variable is the same one where we look it up later.

The problem seems to be that we're doing a JS_ClearScope right after the text is entered into the error console. I've attached a stack trace for where this happens, in case it helps anyone.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Ah, I see.  That's the JS channel loading, and that would clear the scope it's evaluated against.  So yes, this is the JS_ClearScope issue...

I thought we now threw when someone tried to run script against a cleared global?

Comment 11

[Jason Orendorff [:jorendorff]]

In the shell:

var _ = 3;
function f() {
    return _;
}
function g(f) {
    clear(this);
    f();
}
g(f);

Comment 12

[Jason Orendorff [:jorendorff]]

Shorter:

function f() {
    clear(this);
    return f;
}
f();

Comment 13

[Jason Orendorff [:jorendorff]]

In HTML, thanks to bz:

<iframe src="javascript:var x; parent.f = function () { return x; };"
        onload="f()"></iframe>

Comment 14

[Jason Orendorff [:jorendorff]]

bz clarifies that the JS_ClearScope that we do in comment 13 was originally added to help keep memory usage down, and historically, every time someone tries to remove it, we leak.

So, one solution is to fix that nonsense right there.

A quicker hack is to replace JS_ClearScope with a weaker version of itself that just deletes configurable properties and sets writable non-configurable properties to undefined, and see if that is good enough. Maybe that would be good enough.

Comment 15

[Jason Orendorff [:jorendorff]]

Closing. This might be exploitable.

Comment 16

[Boris Zbarsky [:bzbarsky]]

The quicker hack would probably be safe in terms of leaks, yes.  If we can find the "real fix" that's good too, of course.  ;)

Comment 17

[Daniel Veditz [:dveditz]]

bclary sees a lot of this assertion in the wild. Is this a reproducible case of bug 642150, or are those likely to be individual separate cases like bug 605015 (which obviously didn't fix this bug)

Comment 18

[Daniel Veditz [:dveditz]]

Sorry to stick you with this bug, Jason: you seem to understand it the best.

Comment 19

[Daniel Veditz [:dveditz]]

Realistically this is not going to make Macaw.

Comment 20

[Jason Orendorff [:jorendorff]]

Attachment: v1

Here's the obvious patch. Fixes the shell testcase.

bc very kindly agreed to help me test this in the browser.

With luck we could get it in before Tuesday.

Comment 22

[Bob Clary [:bc] (inactive)]

The patch appears to fix the assertions on Mac at least for the 980 urls where I have seen it. I did encounter a couple of new assertions which are unrelated to the patch but which were hidden due to this assertion. It would be really great to get this on to mozilla-central soonest so I can shake out any more assertions or crashes which were also hidden by this assertion as well as to confirm that it is really and truly dead.

Comment 23

[Jason Orendorff [:jorendorff]]

http://hg.mozilla.org/tracemonkey/rev/e8a149ad8bcf

Comment 24

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

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