1255800 - Remove JS_THIS_OBJECT

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Tom S [:evilpie]]

Attachment: Remove JS_THIS_OBJECT from dom/xpconnect

Comment 2

[Tom S [:evilpie]]

Attachment: Remove JS_THIS_OBJECT from mozStorageStatementJSHelper

Hemove JS_THIS_OBJECT

Comment 3

[Tom S [:evilpie]]

Attachment: Remove JS_THIS_OBJECT from js

Comment 4

[Tom S [:evilpie]]

Comment on attachment 8961439 [details] [diff] [review]
Remove JS_THIS_OBJECT from dom/xpconnect

I had to keep computeThis in FunctionForwarder, XPC_WN_Shared_ToString and XPC_WN_CallMethod otherwise I got a lot of orange.

Apparently we have tests that do stuff like `var x = Components.something; x()` , where |this| isn't important, but we should not throw.

Comment 5

[Tom S [:evilpie]]

Comment on attachment 8961440 [details] [diff] [review]
Remove JS_THIS_OBJECT from mozStorageStatementJSHelper

I am really not quite sure how is responsible for this code.

Comment 6

[Jason Orendorff [:jorendorff]]

Comment on attachment 8961441 [details] [diff] [review]
Remove JS_THIS_OBJECT from js

Review of attachment 8961441 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks.

::: js/src/ctypes/CTypes.h
@@ +181,5 @@
>  DeflateStringToUTF8Buffer(JSContext* maybecx, const CharT* src, size_t srclen,
>                            char* dst, size_t* dstlenp);
>  
> +bool
> +IncompatibleThisProto(JSContext* cx, const char* funName, HandleValue actualVal);

I don't see where this one is used. Maybe it can stay `static`.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8961439 [details] [diff] [review]
Remove JS_THIS_OBJECT from dom/xpconnect

>+++ b/dom/bindings/BindingUtils.cpp
>+    JS_ReportErrorASCII(cx, "Called on incompatible |this|");

How about: "QueryInterface called with a non-object |this|"?

>+++ b/dom/plugins/base/nsJSNPRuntime.cpp
>+    ThrowJSExceptionASCII(cx, "Called on incompatible object!");

How about: "plug-in method called with non-object |this|"?

> XPC_WN_Shared_ToString(JSContext* cx, unsigned argc, Value* vp)
>+        JS_ReportErrorASCII(cx, "Called on incompatible |this|");

Please have the error message say _what_ got called.  Presumably toString off an xpconnect object...

>@@ -176,9 +180,11 @@ XPC_WN_DoubleWrappedGetter(JSContext* cx
>+        JS_ReportErrorASCII(cx, "Called on incompatible |this|");

Again, more info about what got called would sure be useful.

>@@ -894,9 +900,12 @@ XPC_WN_CallMethod(JSContext* cx, unsigne
>+        JS_ReportErrorASCII(cx, "Called on incompatible |this|");

Again, if we can say more about what got called it sure would be nice.

I wonder when you actually hit a case where you need to computeThis here.  Do you know, offhand?

>@@ -920,9 +929,11 @@ XPC_WN_GetterSetter(JSContext* cx, unsig
>+        JS_ReportErrorASCII(cx, "Called on incompatible |this|");

Again, better error message would be nice.

r=me

Comment 8

[Tom S [:evilpie]]

Thanks for the quick review. Do I need to figure out the function name somehow in the XPConnect case?

This try push has a lot of failures:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=54f46ebf934a1462e3ba0ed1173b99fca735d5ce&selectedJob=169654667

js/xpconnect/tests/unit/test_bug851895.js (X3) is quite interesting. This is where I got my example from.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> Do I need to figure out the function name somehow in the XPConnect case?

It's just the name (in the JSFunction sense) of the callee in the XPC_WN_CallMethod case.  But I'd be ok with "XPIDL method called on incompatible |this|" and similar for "XPIDL getter or setter ...." and "double-wrapped XPConnect getter ... " or something.   Just unique strings so in case it happens people have some idea where to start looking.

I had missed the example at the end of comment 4.  Thank you for pointing it out.  Alright, then...

Comment 10

[Tom S [:evilpie]]

Oh I just realized that for XPC_WN_CallMethod and XPC_WN_Shared_ToString we use args.computeThis(), which can only fail when boxing a primitive value. (This is of course extremely rare) I think we are ignoring those exception in some places now.

Comment 11

[Tom S [:evilpie]]

Attachment: Make computeThis return a boolean for easier error handling

Comment 12

[Tom S [:evilpie]]

Comment on attachment 8961728 [details] [diff] [review]
Make computeThis return a boolean for easier error handling

I think this is easier to understand and makes it harder to forgot to handle the case when we threw an exception. The possibly only concern is that now, the MutableHandleObject of course doesn't directly alias the this value in the CallArgs.

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8961440 [details] [diff] [review]
Remove JS_THIS_OBJECT from mozStorageStatementJSHelper

Review of attachment 8961440 [details] [diff] [review]:
-----------------------------------------------------------------

Stealing this back, this is fine.

(Please fix the typo in the commit message: Hemove -> Remove)

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf1a120681d0
Remove JS_THIS_OBJECT from js. r=jorendorff
https://hg.mozilla.org/integration/mozilla-inbound/rev/16d254b2fe0d
Remove JS_THIS_OBJECT from mozStorageStatementJSHelper. r=mrbkap
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a6a2971bce3
Remove JS_THIS_OBJECT from dom/xpconnect. r=bz

Comment 15

[Tom S [:evilpie]]

I didn't fix the error message in XPC_WN_CallMethod and XPC_WN_Shared_ToString, because both of these are going away after landing the last patch.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/da734f3cafaf
Fix typo. r=me CLOSED TREE

Comment 17

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/bf1a120681d0
https://hg.mozilla.org/mozilla-central/rev/16d254b2fe0d
https://hg.mozilla.org/mozilla-central/rev/9a6a2971bce3
https://hg.mozilla.org/mozilla-central/rev/da734f3cafaf

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/edcef7625d06
Make computeThis return a boolean for easier error handling. r=jorendorff

Comment 19

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/edcef7625d06