allow non-object values to be used as the this value when invoking IDL callback functions

RESOLVED FIXED in Firefox 39

Status

()

defect
RESOLVED FIXED
4 years ago
4 months ago

People

(Reporter: heycam, Assigned: heycam)

Tracking

(Blocks 1 bug)

Trunk
mozilla39
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

4 years ago
I want to be able to invoke an IDL callback function value with a non-object this value (a value that comes from script, just like the second argument to [].forEach).  That's not possible right now with the two Call() methods defined on codegened CallbackFunction objects.
Assignee

Comment 1

4 years ago
Posted patch patch (obsolete) — Splinter Review
Assignee: nobody → cam
Status: NEW → ASSIGNED
Attachment #8581440 - Flags: review?(peterv)
If we replaced WrapCallThisObject with a WrapCallThisValue that just returns bool and has a MutableHandle<Value> outparam, with a specialization for the T == Rooted<Value> case akin to the one we have for the Rooted<JSObject*> case, then it seems like we wouldn't need the additional codegenned method at all, right?  Better, imo, to put the logic in C++ if we can rather than codegen.

Past that, you need to wrap even non-object values (e.g. strings!).  So you want to use JS_WrapValue in the new method, whether you do it the way you've done it or the way I'm proposing.
Assignee

Comment 3

4 years ago
(In reply to Not doing reviews right now from comment #2)
> If we replaced WrapCallThisObject with a WrapCallThisValue that just returns
> bool and has a MutableHandle<Value> outparam, with a specialization for the
> T == Rooted<Value> case akin to the one we have for the Rooted<JSObject*>
> case, then it seems like we wouldn't need the additional codegenned method
> at all, right?  Better, imo, to put the logic in C++ if we can rather than
> codegen.

Yes that works well, thanks.

I noticed a problem and I'm not sure where it's coming from.  When I try:

  document.fonts.add(new FontFace("x", "url(x)"));
  document.fonts.forEach(function() { console.log(typeof this); console.log(this); }, "abc");

the this value used when invoking the callback function is a String object, rather than a primitive value.  I stepped through into JS::Call, and we still have a primitive value at that point.  Do you know where this conversion into an object is done?  Set.forEach doesn't convert its thisArg into an object.
Assignee

Comment 4

4 years ago
Posted patch patch v2Splinter Review
Attachment #8581440 - Attachment is obsolete: true
Attachment #8581440 - Flags: review?(peterv)
Attachment #8582102 - Flags: review?(peterv)
> the this value used when invoking the callback function is a String object, rather than a
> primitive value. 

That's because the callback function is non-strict.  If you look at http://people.mozilla.org/~jorendorff/es6-draft.html#sec-ecmascript-function-objects-call-thisargument-argumentslist step 6 is what sets up "this" and http://people.mozilla.org/~jorendorff/es6-draft.html#sec-ordinarycallbindthis steps 5/6 check for strict mode and if not replace the value with either the global (if null/undefined) or ToObject(thisArgument) otherwise.

In terms of SpiderMonkey implementation, we do this lazily, when JSOP_THIS is evaluated.  That calls ComputeThis() which will do BoxNonStrictThis() as needed.

> Set.forEach doesn't convert its thisArg into an object.

Sure it will, for a non-strict callback:

  var s = new Set()
  s.add(1)
  s.forEach(function() { alert(typeof this); }, "aaa")

alerts "object".
Assignee

Comment 6

4 years ago
Aha!  Thanks for explaining.  (I think I imagined Set's behaviour after reading the ES spec not too closely, rather than by testing.)
Attachment #8582102 - Flags: review?(peterv) → review+
https://hg.mozilla.org/mozilla-central/rev/8183a5fa421d
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.