Bug 216507

Summary: Element should not set an attribute inside its constructor
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: DOMAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: calvaris, cdumez, changseok, cmarcelo, darin, eric.carlson, esprehn+autocc, ews-watchlist, ggaren, glenn, graouts, gyuyoung.kim, jer.noble, kangil.han, mifenton, philipj, pxlcoder, sergio, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 216528    
Attachments:
Description Flags
Patch
ews-feeder: commit-queue-
Fixed the media tests and addresed Darin's review comments wenson_hsieh: review+

Description Ryosuke Niwa 2020-09-14 17:00:43 PDT
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.
Comment 1 Ryosuke Niwa 2020-09-14 17:05:24 PDT
Created attachment 408764 [details]
Patch
Comment 2 Darin Adler 2020-09-14 17:23:23 PDT
Can we add some assertions to help catch this immediately the first time the code is run?
Comment 3 Ryosuke Niwa 2020-09-14 17:25:51 PDT
(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.
Comment 4 Darin Adler 2020-09-14 17:34:35 PDT
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?
Comment 5 Ryosuke Niwa 2020-09-14 18:37:05 PDT
(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.
Comment 6 Ryosuke Niwa 2020-09-14 19:36:26 PDT
Created attachment 408780 [details]
Fixed the media tests and addresed Darin's review comments
Comment 7 Wenson Hsieh 2020-09-14 21:21:19 PDT
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
> +2020-09-14  Ryosuke Niwa  <rniwa@webkit.org>
> +
> +        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.
Comment 8 Ryosuke Niwa 2020-09-14 22:14:15 PDT
(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
> > +2020-09-14  Ryosuke Niwa  <rniwa@webkit.org>
> > +
> > +        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.
Comment 9 Ryosuke Niwa 2020-09-14 22:17:26 PDT
Committed r267074: <https://trac.webkit.org/changeset/267074>
Comment 10 Radar WebKit Bug Importer 2020-09-14 22:18:16 PDT
<rdar://problem/68900654>