Summary: | Element should not set an attribute inside its constructor | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||
Component: | DOM | Assignee: | 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
Ryosuke Niwa
2020-09-14 17:00:43 PDT
Created attachment 408764 [details]
Patch
Can we add some assertions to help catch this immediately the first time the code is run? (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 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? (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. Created attachment 408780 [details]
Fixed the media tests and addresed Darin's review comments
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. (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. Committed r267074: <https://trac.webkit.org/changeset/267074> |