Status

()

defect
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: jorendorff, Assigned: evilpie)

Tracking

unspecified
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(4 attachments)

No description provided.
Assignee

Updated

a year ago
Assignee: nobody → evilpies
Assignee

Updated

a year ago
Depends on: 888600
Assignee

Updated

a year ago
Attachment #8961441 - Flags: review?(jorendorff)
Assignee

Comment 4

a year ago
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)
Assignee

Comment 5

a year ago
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)
Reporter

Comment 6

a year ago
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+
Assignee

Comment 8

a year ago
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...
Assignee

Comment 10

a year ago
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.
Assignee

Updated

a year ago
Attachment #8961439 - Attachment is obsolete: true
Assignee

Updated

a year ago
Attachment #8961439 - Attachment is obsolete: false
Assignee

Comment 12

a year ago
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)
Assignee

Updated

a year ago
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+

Comment 14

a year ago
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
Assignee

Updated

a year ago
Keywords: leave-open
Assignee

Comment 15

a year ago
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.
Reporter

Updated

a year ago
Attachment #8961728 - Flags: review?(jorendorff) → review+

Comment 18

a year ago
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
Assignee

Updated

a year ago
Keywords: leave-open

Comment 19

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/edcef7625d06
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.