Bug 200427 – WebRTC: got incorrect `this` in negotiationneeded event

• Status: RESOLVED
• Resolution: FIXED
• Component: WebRTC

[zjcqoo]

```html
<!--
UA: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14_5) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/12.1.1 Safari/605.1.15
-->
<script>
const pc = new RTCPeerConnection(null)

pc.onnegotiationneeded = async function(e) {
  console.log(pc === this)  // return false! should be true

  // TypeError:
  // Can only call RTCPeerConnection.createOffer on instances of RTCPeerConnection
  const offer = await this.createOffer()
  console.log('offer:', offer)
}
pc.createDataChannel('x')
</script>
```

[youenn fablet]

Created attachment 375509 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/53923961>

[Darin Adler]

Comment on attachment 375509 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=375509&action=review

> Source/WebCore/bindings/js/JSDOMBuiltinConstructor.h:95
> +typename JSC::JSObject& createJSObjectFromWrapper(JSDOMBuiltinConstructor<JSClass>& constructor, Ref<typename JSClass::DOMWrapped>&& domObject)

Not a big fan of names with "dom" in them. I’d call this "wrappedObject" instead of "domObject" if I was writing the code.

> Source/WebCore/bindings/js/JSDOMBuiltinConstructor.h:101
> +    auto* domObjectPtr = domObject.ptr();
> +    auto wrapper = JSClass::create(getDOMStructure<JSClass>(globalObject.vm(), globalObject), &globalObject, WTFMove(domObject));
> +    cacheWrapper(globalObject.world(), domObjectPtr, wrapper);
> +    return *wrapper;

This looks so identical to createWrapper from JSDOMWrapperCache.h. There’s got to be a way to factor so they share most of their code.

> Source/WebCore/bindings/js/JSDOMBuiltinConstructor.h:104
> +template<typename JSClass> inline

Seems unnecessary that all of these function templates say "inline". I can’t imagine this has any effect one way or another on whether they are inlined nor on whether they are safe to include in a header.

[youenn fablet]

(In reply to Darin Adler from comment #3)
> Comment on attachment 375509 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=375509&action=review
> 
> > Source/WebCore/bindings/js/JSDOMBuiltinConstructor.h:95
> > +typename JSC::JSObject& createJSObjectFromWrapper(JSDOMBuiltinConstructor<JSClass>& constructor, Ref<typename JSClass::DOMWrapped>&& domObject)
> 
> Not a big fan of names with "dom" in them. I’d call this "wrappedObject"
> instead of "domObject" if I was writing the code.

OK

> > Source/WebCore/bindings/js/JSDOMBuiltinConstructor.h:101
> > +    auto* domObjectPtr = domObject.ptr();
> > +    auto wrapper = JSClass::create(getDOMStructure<JSClass>(globalObject.vm(), globalObject), &globalObject, WTFMove(domObject));
> > +    cacheWrapper(globalObject.world(), domObjectPtr, wrapper);
> > +    return *wrapper;
> 
> This looks so identical to createWrapper from JSDOMWrapperCache.h. There’s
> got to be a way to factor so they share most of their code.

Right will do.

> > Source/WebCore/bindings/js/JSDOMBuiltinConstructor.h:104
> > +template<typename JSClass> inline
> 
> Seems unnecessary that all of these function templates say "inline". I can’t
> imagine this has any effect one way or another on whether they are inlined
> nor on whether they are safe to include in a header.

I guess this is copy/paste with the inline methods.

[youenn fablet]

Created attachment 375513 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 375513 [details]
Patch for landing

Clearing flags on attachment: 375513

Committed r248267: <https://trac.webkit.org/changeset/248267>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 375513 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=375513&action=review

> Source/WebCore/bindings/js/JSDOMBuiltinConstructor.h:97
> +    return *createWrapper<typename JSClass::DOMWrapped>(constructor.globalObject(), JSClass::DOMWrapped::create());

I think we can take out the <typename JSClass::DOMWrapped> and everything will still work properly, deduced based on the argument types. Might be worth trying that at some point.

> Source/WebCore/bindings/js/JSDOMBuiltinConstructor.h:111
> +    return context ? createWrapper<typename JSClass::DOMWrapped>(constructor.globalObject(), JSClass::DOMWrapped::create(*context)) : nullptr;

Ditto.