Closed Bug 539782 Opened 15 years ago Closed 7 years ago

XPConnect should warn when user passes undefined in as an argument

Categories

(Core :: XPConnect, enhancement)

x86
All
enhancement
Not set
normal

Tracking

()

RESOLVED INACTIVE

People

(Reporter: WeirdAl, Unassigned)

Details

Attachments

(1 file, 2 obsolete files)

Consider the following: var foo; elem.setAttribute("name", foo); If I call elem.getAttribute("name"), I get back "undefined". This is correct, but as a developer, I don't expect that. I probably introduced a bug when I called setAttribute with an undefined value. I'd like to see a JS warning logged for that scenario: "Argument x is undefined". It looks like JSVAL_IS_VOID(pval) can identify the value as undefined, and JS_ReportWarning can deliver the warning. I think implementing this would be fairly easy. I can't think of any reason why a developer would want to pass undefined into XPConnect (except possibly for testing). If I'm missing something, please let me know.
Attached patch testcase (obsolete) — Splinter Review
i believe that "undefined" is managed by js. and i don't think you'll find any useful place to manage such a warning.
Assignee: nobody → general
Component: XPConnect → JavaScript Engine
QA Contact: xpconnect → general
Attached patch work-in-progress (obsolete) — Splinter Review
(In reply to comment #2) > i don't think you'll find any useful place to manage such a warning. Oh, really? :-) This patch helped me find bug 539980. XPConnect has to convert every single argument from a JS value to a C++ pointer, before calling the native method. There has to be a method where that happens - two actually. One for quickstubs, one for non-quickstubbed methods. I remember stepping through the non-QS'd one many times during debugging. I just haven't gotten that far yet.
Attached patch patch, v1Splinter Review
Assignee: general → ajvincent
Status: NEW → ASSIGNED
Component: JavaScript Engine → XPConnect
QA Contact: general → xpconnect
Comment on attachment 421739 [details] [diff] [review] testcase testcase doesn't work in xpcshell-tests, check-interactive. I think it's due to default warnings settings when invoking xpcshell (but don't quote me on that)
Attachment #421739 - Attachment is obsolete: true
Attachment #421866 - Attachment is obsolete: true
Comment on attachment 421954 [details] [diff] [review] patch, v1 on dbaron's advice, seeking review from peterv for the quickstubs part. Still need to figure out who can review the xpcwrappednative change.
Attachment #421954 - Flags: review?(peterv)
Er... shouldn't the quickstub just throw like the normal XPConnect method does?
Or does the xpconnect method not throw?
Neither of them throws, bz - they both log warnings. That's as much as I wanted to risk here - not logging an error, and not throwing one - on my own. This is not code I normally hack in, and if I'm going to start throwing errors or logging them, I'd better consult with someone who is more of an authority on this code than I am. If peers say I should throw or log an actual error, I'll be more than happy to accomodate them.
Component: XPConnect → JavaScript Engine
> Neither of them throws, bz - they both log warnings. Where does the normal xpconnect method log a warning? Do you mean with your patch? About 60 lines above the code you added, there's a block like this: 2364 if(argc < requiredArgs) { 2365 Throw(NS_ERROR_XPC_NOT_ENOUGH_ARGS, ccx); 2366 goto done; 2367 } so if you happened to call a non-quickstub method with not enough args you wouldn't even reach your code. Which makes me wonder how this patch was tested...
(In reply to comment #10) > so if you happened to call a non-quickstub method with not enough args you > wouldn't even reach your code. > > Which makes me wonder how this patch was tested... Ah, that's what you mean. In the case I'm examining, it's not that the number of arguments is too low, but that the caller literally passed in an undefined value in somewhere among the arguments. I tested it using attachment 421739 [details] [diff] [review], and a debugger attached. (I will probably convert it to a different format, one guaranteed to trigger warnings. Chrome mochitest as a last resort.) Specifically, I tested by directly passing the JS undefined literal in: root.checked = undefined; root.setAttribute("type", undefined); etc. Comment 0 describes what ultimately happened to me in bug 539980: this.setAttribute("value", val.value) - when val.value evaluated to undefined. The warning showed up in that condition as well.
Oh, I see. You're worried about the explicit-undefined case.... I'm not sure we want a warning there, and I'm particularly not sure that we want the cost of an extra function call per argument there in the quickstub.
bz has suggested over IRC making xpc_qsWarnIfUndefined into an inline function which lives only in the header. Yet another fact of C++ (inline functions) I didn't know about. Also, I'd be perfectly fine with putting all this inside #ifdef DEBUG or some other #ifdef block, if necessary.
Component: JavaScript Engine → XPConnect
Comment on attachment 421954 [details] [diff] [review] patch, v1 I'm not convinced we should do this either. Why only do it in the non-traceable versions? >+ argIdx = 0 >+ if i is not None: >+ argIdx = i if isSetter: argIdx = 0 else: argIdx = i >- template = argumentUnboxingTemplates.get(typeName) >+ template = " xpc_qsWarnIfUndefined(cx, ${argVal}, ${argIdx});\n" >+ template += argumentUnboxingTemplates.get(typeName) Write this out directly, instead of adding it to the template. >+void >+xpc_qsWarnIfUndefined(JSContext *cx, jsval v, PRInt32 idx) >+{ >+ if (JSVAL_IS_VOID(v)) >+ { >+ JS_ReportWarning(cx, "Argument %d is undefined", idx); >+ } >+} inline.
Attachment #421954 - Flags: review?(peterv) → review-
(In reply to comment #14) > I'm not convinced we should do this either. What changes, if any, would convince you, bz, and other peers this should be brought in? I've already suggested ifdef DEBUG (or, say, ifdef DEBUG_UNDEFINED_CALLERS), but I heard no response there. > Why only do it in the non-traceable versions? Hmm? I must've missed something. Could you point out the missing code path, please?
Assignee: ajvincent → nobody
Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → INACTIVE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: