Last Comment Bug 690961 - "ASSERTION: Won't check if this is chrome, you want to set aWantsUntrusted to PR_FALSE..." due to optional_argc overflowing 8-bit unsigned
: "ASSERTION: Won't check if this is chrome, you want to set aWantsUntrusted to...
Status: VERIFIED FIXED
[qa!]
: assertion, testcase, verified-beta
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla10
Assigned To: Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
:
Mentors:
Depends on:
Blocks: 326633
  Show dependency treegraph
 
Reported: 2011-09-30 16:15 PDT by Jesse Ruderman
Modified: 2011-12-28 16:19 PST (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
verified
verified
verified
unaffected


Attachments
testcase (200 bytes, text/html)
2011-09-30 16:15 PDT, Jesse Ruderman
no flags Details
stack trace (3.05 KB, text/plain)
2011-09-30 16:17 PDT, Jesse Ruderman
no flags Details
Clamp optional_argc values so that it is never higher than the number of parameters (1.72 KB, patch)
2011-10-08 06:01 PDT, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
mrbkap: review+
asa: approval‑mozilla‑aurora+
asa: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Jesse Ruderman 2011-09-30 16:15:06 PDT
Created attachment 563878 [details]
testcase

###!!! ASSERTION: Won't check if this is chrome, you want to set aWantsUntrusted to PR_FALSE or make the aWantsUntrusted explicit by making aOptionalArgc non-zero.: '!aWantsUntrusted || aOptionalArgc > 1', file content/base/src/nsGenericElement.cpp, line 1078
Comment 1 Jesse Ruderman 2011-09-30 16:17:18 PDT
Created attachment 563880 [details]
stack trace
Comment 2 Boris Zbarsky [:bz] 2011-10-03 21:29:48 PDT
The problem is that aOptionalArgc is a PRUint8.  In this case we have 258 arguments, so argc is 258 and since this function has two non-obvious arguments 256 is passed for aOptionalArgc.  But PRUint8(256) == 0.

Is there a good reason to not just make the optional_argc 32-bit and be done with it?

We could also remove the assertion in question, I suspect...
Comment 3 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-10-04 01:34:22 PDT
The question is, would it be faster to clamp to the maximum number of optional arguments that the function takes, a well predicted branch seems faster than pushing more bytes to the stack.

It's news to me that that the clamping isn't happening already. I wonder if that results in buggy behavior if we do a == check to the maximum number of expected optional arguments somewhere.
Comment 4 Boris Zbarsky [:bz] 2011-10-04 10:22:29 PDT
> would it be faster to clamp to the maximum number of optional arguments that the function
> takes

Faster, probably not.  But that clamping might be the right thing to do, yeah.
Comment 5 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-10-04 15:51:30 PDT
The compiler almost certainly is allocating more than one byte anyways on the stack for this, so this shouldn't change the number of bytes pushed onto the stack.

But yeah, we definitely should clamp here.
Comment 6 Daniel Veditz [:dveditz] 2011-10-05 17:12:18 PDT
Would this just result in us losing the user's arguments? That doesn't sound too scary.
Comment 7 Boris Zbarsky [:bz] 2011-10-05 21:16:38 PDT
Which "this"?  The bug, or the proposed fix?
Comment 8 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-10-08 06:01:33 PDT
Created attachment 565732 [details] [diff] [review]
Clamp optional_argc values so that it is never higher than the number of parameters

This fixes the testcase.  Percolating through try now.
Comment 9 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-10-09 07:02:37 PDT
This is green on try.
Comment 10 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-10-11 05:13:13 PDT
https://hg.mozilla.org/mozilla-central/rev/2e555f1dcdff
Comment 11 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-10-11 05:14:53 PDT
Comment on attachment 565732 [details] [diff] [review]
Clamp optional_argc values so that it is never higher than the number of parameters

I think we should take this patch on any branch we are going to ship a release from in the future.  It's very narrow and low risk and prevents scenarios that could cause problems all over the place.
Comment 12 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-10-12 06:45:39 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/9c11492b5310
https://hg.mozilla.org/releases/mozilla-beta/rev/98e69bdadbd3
Comment 13 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-10-13 11:23:49 PDT
Is this fix verifiable a normal Firefox build using the attached testcase?
Comment 14 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-10-13 11:24:22 PDT
No, you need a debug build.
Comment 15 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-10-13 11:50:07 PDT
qa- based on comment 14. Can someone who is already set up to test this bug please verify the fix?
Comment 16 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-10-13 12:00:45 PDT
FTR, you don't have to build debug builds yourself.  You can download them from tinderbox for non-Windows platforms (e.g. ftp://ftp.mozilla.org/pub/firefox/nightly/2011-10-12-mozilla-central-debug/)

(Getting Windows debug builds to run on your machine requires installing the matching version of the compiler though)
Comment 17 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-10-13 12:02:46 PDT
qa+ per IRC since this should be verifiable easily on non-Windows with the builds from comment 16 (or similar builds on different branches).
Comment 18 Alex Keybl [:akeybl] 2011-10-26 16:01:52 PDT
Please land this on 1.9.2.24 asap. We're looking to go to build by the end of the week (at the latest). Thanks!
Comment 19 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-10-26 16:06:14 PDT
Ah, sorry forgot about this.  I'm heading out for the night, but I'll land it first thing tomorrow.
Comment 20 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-10-27 04:50:52 PDT
Actually, 1.9.2 doesn't even have optional_argc, so we don't need to do anything.
Comment 21 Tracy Walker [:tracy] 2011-12-05 08:40:26 PST
test case doesn't crash 8,9 or 10
Comment 22 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-12-05 09:58:21 PST
There never was a crash here.  See comments 13-17.
Comment 23 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-12-06 15:59:46 PST
Kyle, so using the latest debug builds should verify this fix right?

Tracy, can you confirm if you tested with the debug versions of Firefox 8, 9, 10 in comment 21?

Thanks
Comment 24 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-12-07 06:39:16 PST
Yes, if you load the testcase in a debug build and don't see any assertions (note that assertions do not cause crashes) then this bug is fixed.
Comment 25 Virgil Dicu [:virgil] [QA] 2011-12-14 04:58:03 PST
Mozilla/5.0 (X11; Linux x86_64; rv:9.0) Gecko/20111110 Firefox/9.0
ftp://ftp.mozilla.org/pub/firefox/nightly/2011-12-13-mozilla-beta-debug

Verified in Firefox 9 Beta on Ubuntu 11.10 x86_64 and Mac OS 10.6 with builds from the above link.

Started Firefox from the terminal and loaded the testcase from comment 0. 
No assertions were displayed.
Comment 26 Virgil Dicu [:virgil] [QA] 2011-12-28 04:54:41 PST
Mozilla/5.0 (X11; Linux x86_64; rv:10.0) Gecko/20100101 Firefox/10.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:10.0) Gecko/20100101 Firefox/10.0

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2011-12-28-mozilla-beta-debug/

Verified on Firefox 10 beta1 on Mac OS 10.6 and Ubuntu 11.10. No assertions displayed when running testcase from commment 0.

Note You need to log in before you can comment on or make changes to this bug.