Closed Bug 1255800 Opened 5 years ago Closed 3 years ago

Remove JS_THIS_OBJECT

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: jorendorff, Assigned: evilpie)

References

Details

Attachments

(4 files)

No description provided.
Assignee: nobody → evilpies
Depends on: 888600
Attachment #8961441 - Flags: review?(jorendorff)
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.
Attachment #8961439 - Flags: review?(bzbarsky)
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.
Attachment #8961440 - Flags: review?(mrbkap)
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`.
Attachment #8961441 - Flags: review?(jorendorff) → review+
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
Attachment #8961439 - Flags: review?(bzbarsky) → review+
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.
> 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...
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.
Attachment #8961439 - Attachment is obsolete: true
Attachment #8961439 - Attachment is obsolete: false
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.
Attachment #8961728 - Flags: review?(jorendorff)
Attachment #8961440 - Flags: review?(mrbkap) → review?(continuation)
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)
Attachment #8961440 - Flags: review?(continuation) → review+
Pushed by evilpies@gmail.com:
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
Keywords: leave-open
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.
Attachment #8961728 - Flags: review?(jorendorff) → review+
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/edcef7625d06
Make computeThis return a boolean for easier error handling. r=jorendorff
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/edcef7625d06
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.