Bug 216507 – Element should not set an attribute inside its constructor

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Ryosuke Niwa]

There is a bunch of shadow elements that set attributes inside their constructors.
This isn't safe because parseAttribute could end up trying to store "this" in Ref/RefPtr.

[Ryosuke Niwa]

Created attachment 408764 [details]
Patch

[Darin Adler]

Can we add some assertions to help catch this immediately the first time the code is run?

[Ryosuke Niwa]

(In reply to Darin Adler from comment #2)
> Can we add some assertions to help catch this immediately the first time the
> code is run?

Yup, the patch adds it to Element::setAttributeInternal.

[Darin Adler]

Comment on attachment 408764 [details]
Patch

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

> Source/WebCore/html/FileInputType.cpp:76
> +    static Ref<UploadButtonElement> createInternal(Document&, String value);

const String&

> Source/WebCore/html/HTMLKeygenElement.cpp:53
> +        auto selectElement = adoptRef(*new KeygenSelectElement(document));

How about just "element" for the local variable name?

> Source/WebCore/html/shadow/YouTubeEmbedShadowElement.cpp:36
>  Ref<YouTubeEmbedShadowElement> YouTubeEmbedShadowElement::create(Document& doc)

Rename from "doc" to "document" while touching this?

[Ryosuke Niwa]

(In reply to Darin Adler from comment #4)
> Comment on attachment 408764 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=408764&action=review
> 
> > Source/WebCore/html/FileInputType.cpp:76
> > +    static Ref<UploadButtonElement> createInternal(Document&, String value);
> 
> const String&

Fixed.

> > Source/WebCore/html/HTMLKeygenElement.cpp:53
> > +        auto selectElement = adoptRef(*new KeygenSelectElement(document));
> 
> How about just "element" for the local variable name?

Sure, renamed.

> > Source/WebCore/html/shadow/YouTubeEmbedShadowElement.cpp:36
> >  Ref<YouTubeEmbedShadowElement> YouTubeEmbedShadowElement::create(Document& doc)
> 
> Rename from "doc" to "document" while touching this?

Done.

[Ryosuke Niwa]

Created attachment 408780 [details]
Fixed the media tests and addresed Darin's review comments

[Wenson Hsieh]

Comment on attachment 408780 [details]
Fixed the media tests and addresed Darin's review comments

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

> Source/WebCore/ChangeLog:83
> [phone]  Ryosuke Niwa  <[e-mail]>
> +
> +        Element should not set an attribute inside its constructor
> +        https://bugs.webkit.org/show_bug.cgi?id=216507
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Moved the code to add attributes from element constructors to respective ::create functions
> +        since setting attributes could run code in parseAttribute some of which may try to store
> +        "this" in Ref / RefPtr, which would not be safe before adoptRef is called.

Nit - duplicated ChangeLog entry.

[Ryosuke Niwa]

(In reply to Wenson Hsieh from comment #7)
> Comment on attachment 408780 [details]
> Fixed the media tests and addresed Darin's review comments
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=408780&action=review
> 
> > Source/WebCore/ChangeLog:83
> > [phone]  Ryosuke Niwa  <[e-mail]>
> > +
> > +        Element should not set an attribute inside its constructor
> > +        https://bugs.webkit.org/show_bug.cgi?id=216507
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        Moved the code to add attributes from element constructors to respective ::create functions
> > +        since setting attributes could run code in parseAttribute some of which may try to store
> > +        "this" in Ref / RefPtr, which would not be safe before adoptRef is called.
> 
> Nit - duplicated ChangeLog entry.

Oops fixed.

[Ryosuke Niwa]

Committed r267074: <https://trac.webkit.org/changeset/267074>

[Radar WebKit Bug Importer]

<rdar://problem/68900654>