539782 - XPConnect should warn when user passes undefined in as an argument

Status: RESOLVED INACTIVE

(Core :: XPConnect, enhancement)

Description

[Alex Vincent [:WeirdAl]]

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.

Comment 1

[Alex Vincent [:WeirdAl]]

Attachment: testcase

Comment 2

[timeless]

i believe that "undefined" is managed by js. and i don't think you'll find any useful place to manage such a warning.

Comment 3

[Alex Vincent [:WeirdAl]]

Attachment: work-in-progress

(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.

Comment 4

[Alex Vincent [:WeirdAl]]

Attachment: patch, v1

Comment 5

[Alex Vincent [:WeirdAl]]

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)

Comment 6

[Alex Vincent [:WeirdAl]]

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.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Er... shouldn't the quickstub just throw like the normal XPConnect method does?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Or does the xpconnect method not throw?

Comment 9

[Alex Vincent [:WeirdAl]]

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.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> 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...

Comment 11

[Alex Vincent [:WeirdAl]]

(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.

Comment 12

[Boris Zbarsky [:bzbarsky]]

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.

Comment 13

[Alex Vincent [:WeirdAl]]

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.

Comment 14

[Peter Van der Beken [:peterv]]

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.

Comment 15

[Alex Vincent [:WeirdAl]]

(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?

Comment 16

[BMO Automation]

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.