440473 - crash [@ Decompile][@ js_GetSrcNoteOffset] with firebug/jQuery

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Brian Crowder]

Surfing to the URL specified causes crashes in Fx3 with firebug 1.2b3, while Decompiling a function from jQuery.


In debug, I get earlier assertions:

#0  JS_Assert (s=0x2d2144 "0", file=0x2db718 "/Users/crowder/mozilla/js/src/jsopcode.cpp", ln=2988) at /Users/crowder/mozilla/js/src/jsutil.cpp:63
#1  0x002714d9 in Decompile (ss=0xbfff84e4, pc=0x18b33890 "\b??9", nb=201, nextop=JSOP_NOP) at /Users/crowder/mozilla/js/src/jsopcode.cpp:2988
#2  0x00279017 in DecompileCode (jp=0x171a1900, script=0x18b33780, pc=0x18b33814 "9", len=201, pcdepth=0) at /Users/crowder/mozilla/js/src/jsopcode.cpp:4681
#3  0x0026c25e in js_DecompileFunction (jp=0x171a1900) at /Users/crowder/mozilla/js/src/jsopcode.cpp:4850
#4  0x001e7dcb in JS_DecompileFunction (cx=0x1762ebb0, fun=0x1672eaf0, indent=4) at /Users/crowder/mozilla/js/src/jsapi.cpp:4861
#5  0x1dc1d167 in jsdScript::GetFunctionSource (this=0x18b339c0, aFunctionSource=@0xbfff87c0) at /Users/crowder/mozilla/js/jsd/jsd_xpc.cpp:1282

The output that has already been printed into the sprinter looks like:
    base = 0x17199a50 "    function () {\n        item = $(this);\n        break;\n        $(\"> li:last-child\", item).remove().end();\n"

Which is an anonymous function in the Drupal code.


More coming up....

Comment 1

[Brian Crowder]

If I open the firebug pane to the "net" tab, I'm able to hit an assertion here:

#0  JS_Assert (s=0x2db954 "top < ss->printer->script->depth", file=0x2db718 "/Users/crowder/mozilla/js/src/jsopcode.cpp", ln=886) at /Users/crowder/mozilla/js/src/jsutil.cpp:63
#1  0x0026a198 in PushOff (ss=0xbfff9ad4, off=89, op=JSOP_ZERO) at /Users/crowder/mozilla/js/src/jsopcode.cpp:886
#2  0x00278dce in Decompile (ss=0xbfff9ad4, pc=0x1f9ba1fb ">HE", nb=826, nextop=JSOP_NOP) at /Users/crowder/mozilla/js/src/jsopcode.cpp:4599
#3  0x00279017 in DecompileCode (jp=0x1eba25d0, script=0x1f9b9e00, pc=0x1f9b9f3c "T", len=826, pcdepth=0) at /Users/crowder/mozilla/js/src/jsopcode.cpp:4681
#4  0x0026c25e in js_DecompileFunction (jp=0x1eba25d0) at /Users/crowder/mozilla/js/src/jsopcode.cpp:4850
#5  0x001e7dcb in JS_DecompileFunction (cx=0x191bb750, fun=0x16886850, indent=4) at /Users/crowder/mozilla/js/src/jsapi.cpp:4861
#6  0x1d366167 in jsdScript::GetFunctionSource (this=0x17b4ac30, aFunctionSource=@0xbfff9db0) at /Users/crowder/mozilla/js/jsd/jsd_xpc.cpp:1282


The output in the sprinter during this crash looks like:

    base = 0x6b41b0 "    function (i, elem) {\n"

Comment 2

[Brian Crowder]

If I ignore the assertions hit in comment 0, I eventually crash here:

#0  0x0020ddc6 in js_GetSrcNoteOffset (sn=0x0, which=0) at /Users/crowder/mozilla/js/src/jsemit.cpp:6671
#1  0x00271b24 in Decompile (ss=0xbfff74c4, pc=0x174ac2f5 "\a", nb=244, nextop=JSOP_NOP) at /Users/crowder/mozilla/js/src/jsopcode.cpp:3072
#2  0x00279017 in DecompileCode (jp=0x200810e0, script=0x174ac210, pc=0x174ac2b4 "?", len=244, pcdepth=0) at /Users/crowder/mozilla/js/src/jsopcode.cpp:4681
#3  0x0026c25e in js_DecompileFunction (jp=0x200810e0) at /Users/crowder/mozilla/js/src/jsopcode.cpp:4850
#4  0x001e7dcb in JS_DecompileFunction (cx=0x17f666a0, fun=0x1672b2a0, indent=4) at /Users/crowder/mozilla/js/src/jsapi.cpp:4861
#5  0x1d3c0167 in jsdScript::GetFunctionSource (this=0x174ac490, aFunctionSource=@0xbfff77a0) at /Users/crowder/mozilla/js/jsd/jsd_xpc.cpp:1282

And the sprinter here looks like:

    base = 0x20081160 "    function () {\n        opt = jQuery.extend({}, optall);\n        p;\n        hidden = jQuery(this).is(\":hidden\");\n        self = this;\n"


This resembles a crash-reporter crash linked by shaver from IRC:
http://crash-stats.mozilla.com/report/index/607737dc-3e0d-11dd-baf6-001321b13766

Comment 3

[Brian Crowder]

To reproduce this with firebug, I turned on ALL of the following features:

	Console	Support for Console logging.
	Script	Support for JavaScript debugging.
	Net	Support for Network monitoring.

In the initial firebug setup tab.

Comment 4

[John J. Barton]

Looks like the same as a report from Firebug user:
http://groups.google.com/group/firebug/browse_thread/thread/615f476f9be139ee#

I confirmed the crash using
http://image.baidu.com/i?tn=baiduimage&ct=201326592&lm=-1&cl=2&word=%C1%F5%B7%BC%B7%C6
and hitting the number [2] in the list near the bottom. My stack was:

http://crash-stats.mozilla.com/report/index/3833bec0-3e8e-11dd-9344-001cc45a2ce4?p=1

Comment 5

[Brian Crowder]

Maybe even blocking1.9.1?

Comment 6

[Damon Sicore (:damons)]

Crowder, can you give some more details on why you think this should block?  Looking for:  common usage scenario?  exploitable?  Do you think this should go in 1.9.0.x?  

Comment 7

[Brian Crowder]

I'm not clear on whether it's exploitable or not, but it's bound to be very frustrating for firebug users, as it seems to be a relatively frequent crash, hit on common sites.  Firefox + Firebug + Baidu == crash seems bad enough to block, to me (see comment #4).  I haven't had a chance to look at this full-time, but I've poked at it a bit over the last few days and it's weird (at least, to me, with limited decompiler experience, it is).

Comment 8

[Brian Crowder]

It's also worth mentioning that the code causing the crash in at least the first case (comment 0) is from jQuery, which is widely used.

Comment 9

[Damon Sicore (:damons)]

This is sounding more and more like 1.9.0.1 fodder.  Beltzner, what do you think?

Comment 10

[Brian Crowder]

Alright, here's what I understand so far.

The Decompilation that's occurring in the browser-debugger is getting confused
by what looks like a bogus or empty source-note-cache entry.

As we're decompiling opcodes, we wind up on a JSOP_IFEQ, then try to lookup a
src-note, the lookup descends into this section of js_GetSrcNoteCached:

    if (JS_GSN_CACHE(cx).script == script) {
        JS_METER_GSN_CACHE(cx, hits);
        entry = (GSNCacheEntry *)
                JS_DHashTableOperate(&JS_GSN_CACHE(cx).table, pc,
                                     JS_DHASH_LOOKUP);
        return entry->sn;
    }

The problem is that at this point, somehow, entry->sn is NULL.  If I modify
this to only return entry->sn if entry->sn is !NULL, then the crash disappears.

It's not clear to me how the source note cache is getting misled here, unless
it is because the context coming from the debugger is somehow different than
the context coming from the script that's failing.

Comment 11

[Brian Crowder]

Attachment: tag the cache from script->code, instead of script

This causes the cache to be validated based on its association to the bytecode vector itself, rather than the script pointer, since we sometimes rewrite the bytecode vector (due to traps, especially).

Comment 12

[Brendan Eich [:brendan]]

Comment on attachment 326743 [details] [diff] [review]
tag the cache from script->code, instead of script

>diff -r fc0d7b238e44 js/src/jscntxt.h
>--- a/js/src/jscntxt.h	Tue Jun 24 13:45:12 2008 -0700
>+++ b/js/src/jscntxt.h	Wed Jun 25 11:12:24 2008 -0700
>@@ -61,10 +61,12 @@
> 
> /*
>  * js_GetSrcNote cache to avoid O(n^2) growth in finding a source note for a
>- * given pc in a script.
>+ * given pc in a script.  We use the script->code pointer to tag the cache,

Nit: eschew French spacing in comments.

/be

Comment 13

[Brian Crowder]

changeset:   15530:4aeb5932fc5a

Woops.  Actually landed this before Brendan's nit.  Will fix in hg.

Comment 14

[Brian Crowder]

changeset:   15532:f171c57e016e  (for the comment nit)

Comment 15

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

Not 1.9.0.1 fodder, but maybe 1.9.0.2.

Comment 16

[John J. Barton]

We are sad....

Comment 17

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Why wasn't this 1.9.0.1 fodder?  It's the dominant crash for Firebug users, easy to trigger with jQuery sites. :/

(See comments 7, 8, 9 from a week ago.)

It's too late for 3.0.1 now, but I'd like to know what more this bug needed to be in order to block, I must say...

Comment 18

[Brendan Eich [:brendan]]

Is this really too late? I'm with shaver, there has to be a specific reason for not taking it. We have more than one bit of brain to think about risk and fixes, even in a firedrill situation, which we're not really in.

/be

Comment 19

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

(In reply to comment #18)
> Is this really too late? I'm with shaver, there has to be a specific reason for
> not taking it. We have more than one bit of brain to think about risk and
> fixes, even in a firedrill situation, which we're not really in.

Shaver emailed release-drivers, which was the right thing to do, and we checked with QA and determined that the risk/reward was worth it. Please land ASAP so it can bake.

In the future, to ensure your patch is considered for a branch release, please nominate it for approval and restate (or point to comments) the assessed risk and reward.

(In reply to comment #18)
> not taking it. We have more than one bit of brain to think about risk and
> fixes, even in a firedrill situation, which we're not really in.

In fact, we sort of are. As has been declared in several places, 3.0.1 is a quick turn release in order to hurry out three security fixes. It's not a firedrill since it isn't a "real" zero day situation, but we are treating it as if it were.

Comment 20

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

Comment on attachment 326743 [details] [diff] [review]
tag the cache from script->code, instead of script

a=beltzner, please land before noon PDT today.

Comment 21

[Brian Crowder]

Attachment: the patch for CVS head

Recording for posterity.  The patch applies trivially in cvs.m.o with only a bit of fuzz, but just in case it needs to be backed out, here it is.

jscntxt.h:  3.205
jsscript.c: 3.181

Comment 22

[Brendan Eich [:brendan]]

Beltzner: I don't know if you are arguing, or agreeing. We took the fix (yay, thanks for approving), so we're not in the kind of firedrill where no risk apart from one fix can be tolerated. But it seems that we were acting as if -- why?

/be

Comment 23

[Hasham]

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.1) Gecko/2008070206 Firefox/3.0.1

Verified using given URL and settings indicated in comment #3. No longer crashes.