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

VERIFIED FIXED

Status

()

Core
JavaScript Engine
--
blocker
VERIFIED FIXED
11 years ago
7 years ago

People

(Reporter: Hussam Al-Tayeb, Assigned: Waldo)

Tracking

({crash, regression})

Trunk
x86
All
crash, regression
Points:
---
Bug Flags:
blocking1.9 +
in-testsuite +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

(crash signature, URL)

Attachments

(4 attachments)

(Reporter)

Description

11 years ago
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.

Updated

11 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking-firefox3?
Keywords: crash, regression
Version: unspecified → Trunk
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
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.
Assignee: nobody → general
Component: General → JavaScript Engine
Flags: blocking-firefox3?
OS: Linux → All
Product: Firefox → Core
QA Contact: general → general

Updated

11 years ago
Flags: blocking1.9?
(Assignee)

Comment 3

11 years ago
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.
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 ;-)
(Reporter)

Comment 5

11 years ago
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

11 years ago
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

Updated

11 years ago
Summary: Firefox crashes when I open 'view page info' on a non-blank page → Firefox crashes when I open 'view page info' on a non-blank page [@ js_ValueToNumber

Updated

11 years ago
Summary: Firefox crashes when I open 'view page info' on a non-blank page [@ js_ValueToNumber → Firefox crashes when I open 'view page info' on a non-blank page [@ js_ValueToNumber]
Waldo, this is a dup, or yours for the taking (could be a latent bug exposed by your checkin?).

/be
(Reporter)

Comment 8

11 years ago
(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
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.
Flags: blocking1.9? → blocking1.9+
this crash also happens when clicking on the "Tell me more"-link of "Larry" on a https-site (like bugzilla).
(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
Severity: critical → blocker

Updated

11 years ago
Duplicate of this bug: 412157
(Assignee)

Updated

11 years ago
Assignee: general → jwalden+bmo
(Assignee)

Comment 13

11 years ago
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.
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
(Assignee)

Comment 15

11 years ago
Created attachment 296911 [details] [diff] [review]
Patch
Attachment #296911 - Flags: review?(brendan)
(Assignee)

Comment 16

11 years ago
Comment on attachment 296911 [details] [diff] [review]
Patch

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

s/argc/0/
Comment on attachment 296911 [details] [diff] [review]
Patch

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

/be
Attachment #296911 - Flags: review?(igor)
Attachment #296911 - Flags: review?(brendan)
Attachment #296911 - Flags: review+
Attachment #296911 - Flags: approval1.9+

Comment 18

11 years ago
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. 
Duplicate of this bug: 412255
(Assignee)

Comment 20

11 years ago
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?
Status: NEW → ASSIGNED

Comment 21

11 years ago
(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

11 years ago
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.
(Assignee)

Comment 23

11 years ago
Created attachment 296981 [details] [diff] [review]
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.
Attachment #296981 - Flags: review?(igor)
Attachment #296981 - Flags: review?(brendan)
(Assignee)

Updated

11 years ago
Blocks: 411889

Comment 24

11 years ago
(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

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

See bug  412296.

Updated

11 years ago
Attachment #296981 - Flags: review?(brendan)
Attachment #296981 - Flags: review+
Attachment #296981 - Flags: approval1.9+
(Assignee)

Comment 26

11 years ago
Created attachment 296986 [details] [diff] [review]
With that last change

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

Updated

11 years ago
Attachment #296986 - Flags: review+
Duplicate of this bug: 412335

Comment 28

11 years ago
Created attachment 297126 [details] [diff] [review]
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!
Attachment #297126 - Flags: review?

Comment 29

11 years ago
/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
Flags: in-testsuite+
Flags: in-litmus-

Updated

11 years ago
Attachment #297126 - Flags: review?
(Assignee)

Comment 30

11 years ago
(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.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → VERIFIED

Updated

10 years ago
Attachment #296911 - Flags: review?(igor)

Updated

10 years ago
Attachment #296981 - Flags: review?(igor)
Crash Signature: [@ js_ValueToNumber]
You need to log in before you can comment on or make changes to this bug.