Closed Bug 519926 Opened 11 years ago Closed 11 years ago
Null dereference in [@xpc
_qs DOMString::xpc _qs DOMString] leads to crash [@ xpc _qs DOMString::xpc _qs DOMString(JSContext*, int, int*, xpc _qs DOMString::Stringification Behavior, xpc _qs DOMString::Stringification Behavior)]
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment on attachment 403989 [details] [diff] [review] Proposed patch for xpcquickstubs.cpp Zelgadis, when you request review, you have to request review from a specific person. The easiest way to see who to request review from is to do 'hg log <filename>' and see who works/reviews in the file. In this case jorendorff is the right guy to look at this.
Attachment #403989 - Flags: review? → review?(jorendorff)
Duplicate of this bug: 520223
Whiteboard: [#3 trunk (1.9.3a1pre) topcrash]
There are two bugs here. I feel I have a definite understanding of what [optional] and [Null(Stringify)] are supposed to mean. Stop me if I go wrong: * [optional] means that if the argument is omitted, we behave as if the caller had passed JS `null`. * [Null(Stringify)] means that if we receive JS `null`, we behave as if the caller had passed the string "null". So our IDL appears to say that document.write() without arguments should call nsIDOMHTMLDocument::Write passing the string "null". This is not what we want, so I think our IDL must be wrong. That's bug #1. However, if we *did* have a quickstubbed method and we really did want it to have an [optional, Null(Stringify)] argument, and the caller omitted it, we would want the quick stub to pass the string "null", not crash. That's bug #2. The patch only tries to address bug #2; and it's not the right fix, because the resulting behavior is to create a void string, not the string "null". Cc:ing bz and peterv because it would be best to fix both bugs at once. I hope bz can take this one.
I think Null(stringify) should just be ignored for optional arguments that are not in fact passed in. At least for the time being. That seems to be more or less the idea of the patch, right?
(In reply to comment #8) If "null" is the desired output for not passing anything at all, then my patch is not doing that - your're completely right about this. I didn't have the same understanding of how [optional] is supposed to behave; in fact I just read that whole code for the first time and drew the conclusion that [optional] means "may or may not be filled" rather than "defaults to null if nothing is here". (In reply to comment #9) Yes, that was the idea, but since there seems to be only one code path leading into xpc_qsDOMString::xpc_qsDOMString, it'd also make sense to fix it at a higher level instead. Either way would fix the crash but as jorrendorf said, mine would produce a void string since there's no space allocated at pval where we could write "null" into.
Taking bug to track it; will reassign to patch author when I push it.
So to make it clear, the XPConnect behavior for this idl if no string is passed it to create a void nsAString. So is the quickstubs behavior with this patch. The behavior if explicit |null| is passed is different, since xpconnect doesn't understand the Null annotation. And the general behavior of this method can no longer be expressed in XPConnect.
Duplicate of this bug: 521760
Summary: Null dereference in [@xpc_qsDOMString::xpc_qsDOMString] leads to crash → Null dereference in [@xpc_qsDOMString::xpc_qsDOMString] leads to crash [@ xpc_qsDOMString::xpc_qsDOMString(JSContext*, int, int*, xpc_qsDOMString::StringificationBehavior, xpc_qsDOMString::StringificationBehavior)]
Attachment #405197 - Flags: review?(jorendorff) → review+
Assignee: bzbarsky → zelgadis
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Crash Signature: [@xpc_qsDOMString::xpc_qsDOMString] [@ xpc_qsDOMString::xpc_qsDOMString(JSContext*, int, int*, xpc_qsDOMString::StringificationBehavior, xpc_qsDOMString::StringificationBehavior)]
You need to log in before you can comment on or make changes to this bug.