Bug 149376 – [Web IDL] Add support for [PutForwards=XXX] IDL extended attribute

• Status: RESOLVED
• Resolution: FIXED
• Component: Bindings

[Chris Dumez]

[Web IDL] Add support for [PutForwards=XXX] IDL extended attribute:
https://heycam.github.io/webidl/#PutForwards

As an initial proof of concept, use it for Document.location as per the specification instead of using custom bindings:
https://html.spec.whatwg.org/multipage/dom.html#the-document-object

More attributes can be ported later.

[Chris Dumez]

Created attachment 261590 [details]
Patch

[Darin Adler]

Comment on attachment 261590 [details]
Patch

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

Looks OK, but there is one significant problem.

> Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:3098
> +    auto* forwardedImpl = WTF::getPtr(castedThis->impl().putForwardsAttribute());
> +    if (!forwardedImpl)
> +        return;
> +    auto& impl = *forwardedImpl;
> +    String nativeValue = value.toString(exec)->value(exec);
> +    if (UNLIKELY(exec->hadException()))
> +        return;
> +    impl.setName(nativeValue);

We can see here that we have a possible object lifetime problem. The reason the code uses WTF::getPtr is that the attribute might return a RefPtr, for example. The whole reason a function would return a RefPtr is that it might want to return a new object where the only owner is the RefPtr. So once we put the pointer into a raw pointer and then let the expression fall out of scope, we have a pointer to a deleted object.

This is not a problem when we use the result of getPtr to call a function, because the underlying RefPtr doesn’t get deallocated until the end of the full expression.

I think the best thing to do for this would be to leave out the call to WTF::getPtr; then this won’t compile if the underlying function returns a RefPtr.

[Darin Adler]

Comment on attachment 261590 [details]
Patch

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

>> Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:3098
>> +    impl.setName(nativeValue);
> 
> We can see here that we have a possible object lifetime problem. The reason the code uses WTF::getPtr is that the attribute might return a RefPtr, for example. The whole reason a function would return a RefPtr is that it might want to return a new object where the only owner is the RefPtr. So once we put the pointer into a raw pointer and then let the expression fall out of scope, we have a pointer to a deleted object.
> 
> This is not a problem when we use the result of getPtr to call a function, because the underlying RefPtr doesn’t get deallocated until the end of the full expression.
> 
> I think the best thing to do for this would be to leave out the call to WTF::getPtr; then this won’t compile if the underlying function returns a RefPtr.

Another option would be to generate this code:

    auto a = castedThis->impl().putForwardsAttribute();
    auto* b = WTF::getPtr(a);
    if (!b)
        return;
    auto& impl = *b;

Not sure what "a" and "b" should be named, though.

[Chris Dumez]

Comment on attachment 261590 [details]
Patch

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

>>> Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:3098
>>> +    impl.setName(nativeValue);
>> 
>> We can see here that we have a possible object lifetime problem. The reason the code uses WTF::getPtr is that the attribute might return a RefPtr, for example. The whole reason a function would return a RefPtr is that it might want to return a new object where the only owner is the RefPtr. So once we put the pointer into a raw pointer and then let the expression fall out of scope, we have a pointer to a deleted object.
>> 
>> This is not a problem when we use the result of getPtr to call a function, because the underlying RefPtr doesn’t get deallocated until the end of the full expression.
>> 
>> I think the best thing to do for this would be to leave out the call to WTF::getPtr; then this won’t compile if the underlying function returns a RefPtr.
> 
> Another option would be to generate this code:
> 
>     auto a = castedThis->impl().putForwardsAttribute();
>     auto* b = WTF::getPtr(a);
>     if (!b)
>         return;
>     auto& impl = *b;
> 
> Not sure what "a" and "b" should be named, though.

Good point, although it would be weird to call a setter on an object that is temporary. It was assumed that the implementation would retain the forwarded attribute's object. I will revisit before landing.

[Darin Adler]

Comment on attachment 261590 [details]
Patch

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

>>>> Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:3098
>>>> +    impl.setName(nativeValue);
>>> 
>>> We can see here that we have a possible object lifetime problem. The reason the code uses WTF::getPtr is that the attribute might return a RefPtr, for example. The whole reason a function would return a RefPtr is that it might want to return a new object where the only owner is the RefPtr. So once we put the pointer into a raw pointer and then let the expression fall out of scope, we have a pointer to a deleted object.
>>> 
>>> This is not a problem when we use the result of getPtr to call a function, because the underlying RefPtr doesn’t get deallocated until the end of the full expression.
>>> 
>>> I think the best thing to do for this would be to leave out the call to WTF::getPtr; then this won’t compile if the underlying function returns a RefPtr.
>> 
>> Another option would be to generate this code:
>> 
>>     auto a = castedThis->impl().putForwardsAttribute();
>>     auto* b = WTF::getPtr(a);
>>     if (!b)
>>         return;
>>     auto& impl = *b;
>> 
>> Not sure what "a" and "b" should be named, though.
> 
> Good point, although it would be weird to call a setter on an object that is temporary. It was assumed that the implementation would retain the forwarded attribute's object. I will revisit before landing.

I think that’s why it’s good to use a raw pointer and remove the call to getPtr. It makes sense for all the cases where forwarding makes sense. There’s no reason for the attribute value to return a RefPtr; it should just be a raw pointer. And I think location() does return a raw pointer. Maybe might be nice to support a reference too, but not RefPtr or Ref.

[Chris Dumez]

(In reply to comment #5)
> Comment on attachment 261590 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=261590&action=review
> 
> >>>> Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:3098
> >>>> +    impl.setName(nativeValue);
> >>> 
> >>> We can see here that we have a possible object lifetime problem. The reason the code uses WTF::getPtr is that the attribute might return a RefPtr, for example. The whole reason a function would return a RefPtr is that it might want to return a new object where the only owner is the RefPtr. So once we put the pointer into a raw pointer and then let the expression fall out of scope, we have a pointer to a deleted object.
> >>> 
> >>> This is not a problem when we use the result of getPtr to call a function, because the underlying RefPtr doesn’t get deallocated until the end of the full expression.
> >>> 
> >>> I think the best thing to do for this would be to leave out the call to WTF::getPtr; then this won’t compile if the underlying function returns a RefPtr.
> >> 
> >> Another option would be to generate this code:
> >> 
> >>     auto a = castedThis->impl().putForwardsAttribute();
> >>     auto* b = WTF::getPtr(a);
> >>     if (!b)
> >>         return;
> >>     auto& impl = *b;
> >> 
> >> Not sure what "a" and "b" should be named, though.
> > 
> > Good point, although it would be weird to call a setter on an object that is temporary. It was assumed that the implementation would retain the forwarded attribute's object. I will revisit before landing.
> 
> I think that’s why it’s good to use a raw pointer and remove the call to
> getPtr. It makes sense for all the cases where forwarding makes sense.
> There’s no reason for the attribute value to return a RefPtr; it should just
> be a raw pointer. And I think location() does return a raw pointer. Maybe
> might be nice to support a reference too, but not RefPtr or Ref.

Well, WTF::getPtr() was also useful if the implementation returned a C++ reference, which it can (and should if the pointer cannot be null).

[Chris Dumez]

Created attachment 261595 [details]
Patch

[Chris Dumez]

Comment on attachment 261590 [details]
Patch

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

>>>>>> Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:3098
>>>>>> +    impl.setName(nativeValue);
>>>>> 
>>>>> We can see here that we have a possible object lifetime problem. The reason the code uses WTF::getPtr is that the attribute might return a RefPtr, for example. The whole reason a function would return a RefPtr is that it might want to return a new object where the only owner is the RefPtr. So once we put the pointer into a raw pointer and then let the expression fall out of scope, we have a pointer to a deleted object.
>>>>> 
>>>>> This is not a problem when we use the result of getPtr to call a function, because the underlying RefPtr doesn’t get deallocated until the end of the full expression.
>>>>> 
>>>>> I think the best thing to do for this would be to leave out the call to WTF::getPtr; then this won’t compile if the underlying function returns a RefPtr.
>>>> 
>>>> Another option would be to generate this code:
>>>> 
>>>>     auto a = castedThis->impl().putForwardsAttribute();
>>>>     auto* b = WTF::getPtr(a);
>>>>     if (!b)
>>>>         return;
>>>>     auto& impl = *b;
>>>> 
>>>> Not sure what "a" and "b" should be named, though.
>>> 
>>> Good point, although it would be weird to call a setter on an object that is temporary. It was assumed that the implementation would retain the forwarded attribute's object. I will revisit before landing.
>> 
>> I think that’s why it’s good to use a raw pointer and remove the call to getPtr. It makes sense for all the cases where forwarding makes sense. There’s no reason for the attribute value to return a RefPtr; it should just be a raw pointer. And I think location() does return a raw pointer. Maybe might be nice to support a reference too, but not RefPtr or Ref.
> 
> Well, WTF::getPtr() was also useful if the implementation returned a C++ reference, which it can (and should if the pointer cannot be null).

I dropped the call to WTF::getPtr() for now. We'll see about supporting implementation methods returning C++ references if it turns out it is needed by any of the PutForwards attributes.

[WebKit Commit Bot]

Comment on attachment 261595 [details]
Patch

Clearing flags on attachment: 261595

Committed r190023: <http://trac.webkit.org/changeset/190023>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

This introduced a use-after-free detected by ASan and GuardMalloc. Will roll out, as memory corruption on regression tests is especially not OK. Will e-mail an ASan violation log to Chris.

[Alexey Proskuryakov]

Chris will roll out himself if this is not an easy fix.