[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.
Created attachment 261590 [details] Patch
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.
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.
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.
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.
(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).
Created attachment 261595 [details] Patch
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.
Comment on attachment 261595 [details] Patch Clearing flags on attachment: 261595 Committed r190023: <http://trac.webkit.org/changeset/190023>
All reviewed patches have been landed. Closing bug.
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.
Chris will roll out himself if this is not an easy fix.