Closed
Bug 1255800
Opened 9 years ago
Closed 7 years ago
Remove JS_THIS_OBJECT
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: jorendorff, Assigned: evilpie)
References
Details
Attachments
(4 files)
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → evilpies
Assignee | ||
Comment 1•7 years ago
|
||
Assignee | ||
Comment 2•7 years ago
|
||
Hemove JS_THIS_OBJECT
Assignee | ||
Comment 3•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Attachment #8961441 -
Flags: review?(jorendorff)
Assignee | ||
Comment 4•7 years 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•7 years 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•7 years 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 7•7 years ago
|
||
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•7 years 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.
Comment 9•7 years ago
|
||
> 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•7 years 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 | ||
Comment 11•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Attachment #8961439 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8961439 -
Attachment is obsolete: false
Assignee | ||
Comment 12•7 years 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•7 years ago
|
Attachment #8961440 -
Flags: review?(mrbkap) → review?(continuation)
Comment 13•7 years ago
|
||
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•7 years 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•7 years ago
|
Keywords: leave-open
Assignee | ||
Comment 15•7 years 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.
Comment 16•7 years ago
|
||
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/da734f3cafaf
Fix typo. r=me CLOSED TREE
Comment 17•7 years ago
|
||
bugherder |
Reporter | ||
Updated•7 years ago
|
Attachment #8961728 -
Flags: review?(jorendorff) → review+
Comment 18•7 years 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•7 years ago
|
Keywords: leave-open
Comment 19•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in
before you can comment on or make changes to this bug.
Description
•