Bug 161423 - Add the check for reentrancy to CustomElementRegistry
Summary: Add the check for reentrancy to CustomElementRegistry
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks: 154907
  Show dependency treegraph
 
Reported: 2016-08-30 22:09 PDT by Ryosuke Niwa
Modified: 2016-08-31 12:33 PDT (History)
5 users (show)

See Also:


Attachments
Adds the check (15.46 KB, patch)
2016-08-30 22:17 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Updated for ToT (15.46 KB, patch)
2016-08-30 22:25 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Updated for ToT (15.48 KB, patch)
2016-08-30 23:16 PDT, Ryosuke Niwa
koivisto: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews122 for ios-simulator-elcapitan-wk2 (8.18 MB, application/zip)
2016-08-31 00:48 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2016-08-30 22:09:10 PDT
Add the "element definition is running" flag to CustomElementRegistry:
https://html.spec.whatwg.org/multipage/scripting.html#element-definition-is-running
and use it in customElements.define.
Comment 1 Ryosuke Niwa 2016-08-30 22:17:50 PDT
Created attachment 287482 [details]
Adds the check
Comment 2 Ryosuke Niwa 2016-08-30 22:25:42 PDT
Created attachment 287486 [details]
Updated for ToT
Comment 3 Ryosuke Niwa 2016-08-30 23:16:01 PDT
Created attachment 287490 [details]
Updated for ToT
Comment 4 Ryosuke Niwa 2016-08-30 23:38:11 PDT
Comment on attachment 287490 [details]
Updated for ToT

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

> LayoutTests/fast/custom-elements/CustomElementRegistry.html:21
>  test(function () {
> +    assert_throws({'name': 'TypeError'}, function () { customElements.define('badname', 1); },

I changed the order of tests to match the order of steps in https://html.spec.whatwg.org/multipage/scripting.html#dom-customelementregistry-define
Comment 5 Build Bot 2016-08-31 00:48:16 PDT
Comment on attachment 287490 [details]
Updated for ToT

Attachment 287490 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1978874

New failing tests:
fast/scrolling/ios/scroll-events-back-forward.html
Comment 6 Build Bot 2016-08-31 00:48:20 PDT
Created attachment 287502 [details]
Archive of layout-test-results from ews122 for ios-simulator-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-elcapitan-wk2  Platform: Mac OS X 10.11.5
Comment 7 Antti Koivisto 2016-08-31 12:18:12 PDT
Comment on attachment 287490 [details]
Updated for ToT

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

> Source/WebCore/dom/CustomElementRegistry.h:53
> +class ElementDefinitionIsRunningTemporaryChange : public TemporaryChange<bool> {
> +public:
> +    ElementDefinitionIsRunningTemporaryChange(CustomElementRegistry&);
> +};

I'm not a big fan of inheriting from basic types. Does this really look better than using TemporaryChange<bool> directly?
Comment 8 Ryosuke Niwa 2016-08-31 12:29:35 PDT
(In reply to comment #7)
> Comment on attachment 287490 [details]
> Updated for ToT
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=287490&action=review
> 
> > Source/WebCore/dom/CustomElementRegistry.h:53
> > +class ElementDefinitionIsRunningTemporaryChange : public TemporaryChange<bool> {
> > +public:
> > +    ElementDefinitionIsRunningTemporaryChange(CustomElementRegistry&);
> > +};
> 
> I'm not a big fan of inheriting from basic types. Does this really look
> better than using TemporaryChange<bool> directly?

Will add bool& member function instead per discussion on IRC.
Comment 9 Ryosuke Niwa 2016-08-31 12:33:03 PDT
Committed r205261: <http://trac.webkit.org/changeset/205261>