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

VERIFIED FIXED

Status

()

defect
P2
critical
VERIFIED FIXED
11 years ago
8 years ago

People

(Reporter: crowderbt, Assigned: crowderbt)

Tracking

({crash, verified1.9.0.1})

Trunk
Points:
---
Bug Flags:
wanted1.9.1 +
blocking1.9.0.1 -
wanted1.9.0.x +
in-testsuite -
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [firebug-p1], crash signature, )

Attachments

(2 attachments)

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

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
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.
Severity: normal → critical
Keywords: crash
Summary: crash (@ Decompile) with firebug/jQuery → crash [@ Decompile][@ js_GetSrcNoteOffset] with firebug/jQuery
Maybe even blocking1.9.1?
Assignee: general → crowder
Flags: wanted1.9.1?
Priority: -- → P2
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?  
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).
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.
This is sounding more and more like 1.9.0.1 fodder.  Beltzner, what do you think?
Flags: blocking1.9.0.1?
Whiteboard: [firebug-p1]
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.
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).
Attachment #326743 - Flags: review?(igor)
Attachment #326743 - Flags: review?(igor) → review+
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
changeset:   15530:4aeb5932fc5a

Woops.  Actually landed this before Brendan's nit.  Will fix in hg.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
changeset:   15532:f171c57e016e  (for the comment nit)
Not 1.9.0.1 fodder, but maybe 1.9.0.2.
Flags: wanted1.9.1?
Flags: wanted1.9.1+
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.1?
Flags: blocking1.9.0.1-
Whiteboard: [firebug-p1] → [firebug-p1][1.9.0.2?]
We are sad....
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...
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
Attachment #326743 - Flags: approval1.9.0.1?
(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 on attachment 326743 [details] [diff] [review]
tag the cache from script->code, instead of script

a=beltzner, please land before noon PDT today.
Attachment #326743 - Flags: approval1.9.0.1? → approval1.9.0.1+
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
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
Keywords: fixed1.9.0.1
Whiteboard: [firebug-p1][1.9.0.2?] → [firebug-p1]
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.
Status: RESOLVED → VERIFIED
Flags: in-testsuite-
Flags: in-litmus-
Crash Signature: [@ Decompile] [@ js_GetSrcNoteOffset]
You need to log in before you can comment on or make changes to this bug.