Closed Bug 690961 Opened 10 years ago Closed 10 years ago
"ASSERTION: Won't check if this is chrome, you want to set a
Wants Untrusted to PR _FALSE ..." due to optional _argc overflowing 8-bit unsigned
200 bytes, text/html
3.05 KB, text/plain
1.72 KB, patch
|Details | Diff | Splinter Review|
###!!! 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
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...
Component: DOM → XPConnect
QA Contact: general → xpconnect
Summary: "ASSERTION: Won't check if this is chrome, you want to set aWantsUntrusted to PR_FALSE..." → "ASSERTION: Won't check if this is chrome, you want to set aWantsUntrusted to PR_FALSE..." due to optional_argc overflowing 8-bit unsigned
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.
> 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.
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.
Would this just result in us losing the user's arguments? That doesn't sound too scary.
Which "this"? The bug, or the proposed fix?
Assignee: nobody → khuey
This fixes the testcase. Percolating through try now.
Attachment #565732 - Flags: review?(mrbkap)
This is green on try.
Attachment #565732 - Flags: review?(mrbkap) → review+
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
OS: Mac OS X → All
Hardware: x86_64 → All
Target Milestone: --- → mozilla10
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.
Is this fix verifiable a normal Firefox build using the attached testcase?
No, you need a debug build.
qa- based on comment 14. Can someone who is already set up to test this bug please verify the fix?
Whiteboard: [qa?] → [qa-]
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)
qa+ per IRC since this should be verifiable easily on non-Windows with the builds from comment 16 (or similar builds on different branches).
Whiteboard: [qa-] → [qa+]
Please land this on 220.127.116.11 asap. We're looking to go to build by the end of the week (at the latest). Thanks!
Ah, sorry forgot about this. I'm heading out for the night, but I'll land it first thing tomorrow.
Actually, 1.9.2 doesn't even have optional_argc, so we don't need to do anything.
test case doesn't crash 8,9 or 10
Status: RESOLVED → VERIFIED
There never was a crash here. See comments 13-17.
Status: VERIFIED → RESOLVED
Closed: 10 years ago → 10 years ago
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
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.
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.
Whiteboard: [qa+] → [qa+], [qa!:9]
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.
Status: RESOLVED → VERIFIED
Whiteboard: [qa+], [qa!:9] → [qa!], [qa!:9] [qa!:10]
You need to log in before you can comment on or make changes to this bug.