412068 - Firefox crashes when I open 'view page info' on a non-blank page [@ js_ValueToNumber]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Hussam Al-Tayeb]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b3pre) Gecko/2008011215 Firefox/3.0b3pre
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b3pre) Gecko/2008011215 Firefox/3.0b3pre

Firefox crashes when I open 'view page info' on a non-blank page.

Reproducible: Always

Steps to Reproduce:
1. Open http://www.google.com
2. Right click on the page.
3. Select 'View Page Info'.
Actual Results:  
Firefox crashes.

Comment 1

[Steve England [:stevee]]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2008011205 Minefield/3.0b3pre ID:2008011205

I see this too.

Works: 20080111_1445_firefox-3.0b3pre.en-US.win32
Crash: 20080111_1522_firefox-3.0b3pre.en-US.win32

Checkins to module PhoenixTinderbox between 2008-01-11 14:45 and 2008-01-11 15:21 : 
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1200091500&maxdate=1200093719

bp-7585dc8f-c11b-11dc-b1c8-001a4bd43e5c
bp-88cf1b14-c11a-11dc-a27b-001a4bd43e5c
bp-3a374d6e-c11a-11dc-9003-001a4bd43ed6

Comment 2

[Steve England [:stevee]]

From the range, I don't think this crash could be caused by bug 408301 since that bug was backed out for the 2008011205 build I'm on, and I still see the crashing. That leaves one of two javascript bugs the more likely to have caused this, so I'm punting this over to the JS Engine component.

Comment 3

[Jeff Walden [:Waldo]]

That range is missing a thinko I fixed in the next checkin outside that range -- try again with a build that picked up the next change.  It'd be more obvious if I had a stack to go from, but none of the crash report links work for me.

Comment 4

[Steve England [:stevee]]

Jeff, as I said in comment 1 and 2, I see the crash with the 2008011205 build. It looks like your follow-up thinko was landed at 2008-01-11 16:12 which was hours before today's daily build (2008011205) was done.

And yes, the crash-stats seem to be taking their sweet time ;-)

Comment 5

[Hussam Al-Tayeb]

I started a new build 5 minutes ago. I'll post back if the crash remains. 
Regarding a stack trace, I did build with --enable-debug but got not debug symbols. Am I missing anything?

Comment 6

[Brian Polidoro]

Crash report is there now:
bp-88cf1b14-c11a-11dc-a27b-001a4bd43e5c

Frame  	Signature  	Source
0 	js_ValueToNumber 	
1 	js_ValueToECMAInt32 	
2 	js_IntToCString 	
3 	js_IntToCString 	
4 	js_Interpret 	
5 	js_Invoke 	
6 	js_InternalInvoke 	
7 	JS_CallFunctionValue 	
8 	nsJSContext::CallEventHandler(nsISupports*, void*, void*, nsIArray*, nsIVariant**) 	mozilla/dom/src/base/nsJSEnvironment.cpp:1967
9 	nsJSEventListener::HandleEvent(nsIDOMEvent*) 	mozilla/dom/src/events/nsJSEventListener.cpp:235
10 	nsEventListenerManager::HandleEventSubType(nsListenerStruct*, nsIDOMEventListener*, nsIDOMEvent*, nsISupports*, unsigned int) 	mozilla/content/events/src/nsEventListenerManager.cpp:1081
11 	nsEventListenerManager::HandleEvent(nsPresContext*, nsEvent*, nsIDOMEvent**, nsISupports*, unsigned int, nsEventStatus*) 	mozilla/content/events/src/nsEventListenerManager.cpp:1183
12 	nsEventTargetChainItem::HandleEvent(nsEventChainPostVisitor&, unsigned int) 	mozilla/content/events/src/nsEventDispatcher.cpp:206
13 	nsEventTargetChainItem::HandleEventTargetChain(nsEventChainPostVisitor&, unsigned int, nsDispatchingCallback*) 	mozilla/content/events/src/nsEventDispatcher.cpp:264
14 	nsEventDispatcher::Dispatch(nsISupports*, nsPresContext*, nsEvent*, nsIDOMEvent*, nsEventStatus*, nsDispatchingCallback*) 	mozilla/content/events/src/nsEventDispatcher.cpp:479
15 	DocumentViewerImpl::LoadComplete(unsigned int) 	mozilla/layout/base/nsDocumentViewer.cpp:979
16 	nsDocShell::EndPageLoad(nsIWebProgress*, nsIChannel*, unsigned int) 	mozilla/docshell/base/nsDocShell.cpp:5028
17 	nsWebShell::EndPageLoad(nsIWebProgress*, nsIChannel*, unsigned int) 	mozilla/docshell/base/nsWebShell.cpp:1014
18 	nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, unsigned int) 	mozilla/docshell/base/nsDocShell.cpp:4928

Comment 7

[Brendan Eich [:brendan]]

Waldo, this is a dup, or yours for the taking (could be a latent bug exposed by your checkin?).

/be

Comment 8

[Hussam Al-Tayeb]

(In reply to comment #5)
> I started a new build 5 minutes ago. I'll post back if the crash remains. 
> Regarding a stack trace, I did build with --enable-debug but got not debug
> symbols. Am I missing anything?
> 

Sorry for taking too much to reply. it is still in my build

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Yep, easily reproducible -- exact same stack for me.  Setting 1.9+, though I bet this'll probably get knocked out trivially by Waldo or someone else tomorrow or monday.

Comment 10

[(mostly gone) XtC4UaLL [:xtc4uall]]

this crash also happens when clicking on the "Tell me more"-link of "Larry" on a https-site (like bugzilla).

Comment 11

[Peter van der Woude [:Peter6]]

(In reply to comment #10)
> this crash also happens when clicking on the "Tell me more"-link of "Larry" on
> a https-site (like bugzilla).
> 

indeed

Comment 13

[Jeff Walden [:Waldo]]

The stack in comment 6 is mangled; js_IntToCString is a leaf function.  Might want to have the Breakbag/Airpad people look into that, assuming it's not a dup.

So, the problem is easy to explain, and I'm too lazy to explain twice:

<Waldo> num_toLocaleString?
<Waldo> no args
<Waldo> no extra spaces for temporary rooting
<Waldo> but it calls num_toString
<Waldo> which takes an argument
<Waldo> so toLocaleString accesses outside of the usable range of |vp|
<Waldo> and sadly, it seems I can't jst change the minargs argument to JS_FN
<Waldo> because it appears we have a JS_ASSERT(minargs <= nargs) for fast natives

I'm still trying to decide what's the right thing to do for the last two lines; maybe a temp root or something.

Comment 14

[Brendan Eich [:brendan]]

Yes, fast-natives used to support local GC roots (guaranteed extra args, per the extra field in JSFunctionSpec). Bug 397239 revised and simplified SpiderMonkey to remove this feature in common among fast and slow natives. You must use a tvr now.

/be

Comment 15

[Jeff Walden [:Waldo]]

Attachment: Patch

Comment 16

[Jeff Walden [:Waldo]]

Comment on attachment 296911 [details] [diff] [review]
Patch

>+    ok = num_toString(cx, argc, roots);

s/argc/0/

Comment 17

[Brendan Eich [:brendan]]

Comment on attachment 296911 [details] [diff] [review]
Patch

Sigh. Igor should take a look too, he may have a better idea.

/be

Comment 18

[Igor Bukanov]

I could be missing something, but given the current:

static JSFunctionSpec number_methods[] = {
...
    JS_FN(js_toString_str,       num_toString,       0,0,JSFUN_THISP_NUMBER),
    JS_FN(js_toLocaleString_str, num_toLocaleString, 0,0,JSFUN_THISP_NUMBER),
...
};

it is safe to call num_toString from num_toLocaleString with the arguments passed to num_toLocaleString. num_toString always checks argc != 0 before accessing the optional base argument.

Thus the bug must be with something else. 

Comment 20

[Jeff Walden [:Waldo]]

Um, are you looking at the most recent source?  I clearly removed the argc != 0
check in revision 3.96:

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/js/src/jsnum.c&rev=3.98&mark=258#245

Speaking of that, we always do have the option of just readding that check and
changing the num_toString argument in num_toLocaleString from |argc| to 0,
easily the simplest option for fixing this bug -- maybe that's better than this
mess?

Comment 21

[Igor Bukanov]

(In reply to comment #20)
> Um, are you looking at the most recent source?  I clearly removed the argc != 0
> check in revision 3.96:

My bad, I indeed have not updated my tree.

> ... we always do have the option of just readding that check and
> changing the num_toString argument in num_toLocaleString from |argc| to 0,
> easily the simplest option for fixing this bug...

In my view that would be the best solution. 

Comment 22

[Igor Bukanov]

Given how hard it would be to spot bugs like this it would be better to remove the minimal arg support from fast native altogether. That would remove the need to an extra checks when calling the fast native for the price of the explicit checks in the native code.

Comment 23

[Jeff Walden [:Waldo]]

Attachment: Restore the argc check, s/argc/0/, test

Okay, let's go with restoring the argc check.

Another thing to note: the argc check is actually necessary to restore correct behavior to toLocaleString when called with a first argument, which before my patches here would have been interpreted as a radix -- but the spec explicitly doesn't say to do this (arg reserved presumably for locale info).  Too bad we didn't have any regression tests for the method...

I think I like removing minargs from fast natives altogether; it only seems useful if you have situations like what happened here, except reversed.  I doubt those are all that common, and it seems like we'd double-pay in branches in at least some cases where minargs < argc.  Fodder for a new bug, I think; feel free to file and assign to me.

Comment 24

[Igor Bukanov]

(In reply to comment #23)
> Created an attachment (id=296981) [details]
> Restore the argc check, s/argc/0/, test

The patch should also change JS_FN declaration for num_toString in number_methods to state 0 minargs.

Comment 25

[Igor Bukanov]

(In reply to comment #23)
> Fodder for a new bug, I think; feel free to file and assign to me.

See bug  412296.

Comment 26

[Jeff Walden [:Waldo]]

Attachment: With that last change

I'll commit this later today -- no real rush beyond getting it in the next nightly.

Comment 28

[Bob Clary [:bc] (inactive)]

Attachment: testcase patch

Waldo, you had a mismatch in quotes in that test. Also, when you add a js test, please minus the in-litmus flag and plus the in-testsuite flag. Thanks!

Comment 29

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/ecma_3/Number/15.7.4.3-01.js,v  <--  15.7.4.3-01.js
new revision: 1.2; previous revision: 1.1

Comment 30

[Jeff Walden [:Waldo]]

(In reply to comment #28)
> Waldo, you had a mismatch in quotes in that test. Also, when you add a js test,
> please minus the in-litmus flag and plus the in-testsuite flag. Thanks!

Oops; I could have sworn I tested that before committing.  As for the flags, I hadn't flipped them because after committing I was too tired to stay up to see that the commit stuck, and I figured it made more sense to get it in for the nightly than to wait any longer.

Anyway, FIXED.

Comment 31

[Marcia Knous [:marcia]]

verified fixed using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b3pre) Gecko/2008011504 Minefield/3.0b3pre. Also verified on the nightly on Win Vista Ultimate.