1275835 - Implement CustomElementsRegistry define function

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[John Dai[:jdai]]

According to session 4.13.4[1], we need to implement CustomElementsRegistry define function, which are window.customElements.define(name, constructor) and window.customElements.define(name, constructor, { extends: baseLocalName }).


[1]https://html.spec.whatwg.org/multipage/scripting.html#element-definition

Comment 1

[Andrew Overholt [:overholt]]

(I'm marking all the Custom Elements bugs as "backlog" but that's just to indicate they're not something we're fixing urgently and no comment on priority or anything.)

Comment 2

[Edgar Chen [:edgar]]

Attachment: WIP patch, v1

Just attach WIP patch which checks the prototype. There are still a lot of works need to be done.

Comment 3

[Edgar Chen [:edgar]]

Attachment: WIP patch, v2

Comment 4

[Edgar Chen [:edgar]]

Attachment: Part 1: Move custom element codes from nsDocument to CustomElementsRegistry, v1

Comment 5

[Edgar Chen [:edgar]]

Comment on attachment 8774612 [details] [diff] [review]
Part 1: Move custom element codes from nsDocument to CustomElementsRegistry, v1

Review of attachment 8774612 [details] [diff] [review]:
-----------------------------------------------------------------

Hi William,
In this bug I am going to implement window.customElements.define() [1].
Since we already have existing codes for custom elements, I plan to reuse them and adjust data-structure/flow to follow the latest spec.

The very first step I am planing to do is moving the data structure from nsDocument to CustomElementsRegistry (As this part1 patch).
And then implement window.customElements.define() with some adjustment on data-structure (this will happen in subsequent part).

I am still working on subsequent part, will attach soon.
But I would like to have your feedback about part1 first.

Here is some details about this part1 patch,
1. Definition/candidate/processingStack now is associated with a window, not a document.
2. I keep the data-structure and call flow the same as mush as possible.
3. Most of the behavior are the same as before [2], except the [3].
   But I think this change make sense because in the spec [4] a element created by a document without docshell won't
   run the custom element setup, so createdCallback won't be triggered in such case.

Please let me know if anything is unclear. Thank you.

[1] https://html.spec.whatwg.org/multipage/scripting.html#dom-customelementsregistry-define
[2] Verified by mochitest tests.
[3] Please see test_document_shared_registry.html changes in this patch.
[4] https://dom.spec.whatwg.org/#dom-document-createelement

Comment 6

[Edgar Chen [:edgar]]

Attachment: Part 1-2: (should be squashed with Part 1 together)

Comment 7

[Edgar Chen [:edgar]]

Attachment: Part 2: Implement CustomElementsRegistry define function, v1

Comment 8

[Edgar Chen [:edgar]]

These patches aren't perfect enough for review.
But they can show you what the whole picture looks like and what I am going to do in this bug.

Comment 9

[William Chen [:wchen]]

Comment on attachment 8775095 [details] [diff] [review]
Part 2: Implement CustomElementsRegistry define function, v1

Review of attachment 8775095 [details] [diff] [review]:
-----------------------------------------------------------------

Overall, it looks like you are on the right track.

::: dom/base/CustomElementsRegistry.cpp
@@ +612,5 @@
> +    // its subclass is not allowed.
> +    JS::Rooted<JS::Value> prototypev(cx);
> +    if (!JS_GetProperty(cx, constructorUnwrapped, "prototype", &prototypev)) {
> +      JS_ClearPendingException(cx);
> +      aRv.Throw(NS_ERROR_UNEXPECTED);

I don't think we should throw here. The exception thrown wouldn't match the exception thrown in the spec by step 13.

@@ +622,5 @@
> +      return;
> +    }
> +
> +    constructorPrototype = &prototypev.toObject();
> +    if (IsDOMIfaceAndProtoClass(js::GetObjectClass(constructorPrototype))) {

The duplicate code should be factored out.

@@ +736,5 @@
> +
> +    JS::Rooted<JS::Value> connectedCallbackValue(cx);
> +    if (!JS_GetProperty(cx, constructorPrototype, "connectedCallback", &connectedCallbackValue)) {
> +      JS_ClearPendingException(cx);
> +      aRv.Throw(NS_ERROR_UNEXPECTED);

Spec says to rethrow any exceptions we get from getting the callbacks.

::: dom/base/CustomElementsRegistry.h
@@ +100,5 @@
>    typedef CustomElementHashKey *KeyType;
>    typedef const CustomElementHashKey *KeyTypePointer;
>  
> +  CustomElementHashKey(nsIAtom *aAtom)
> +    : mAtom(aAtom)

Since we don't need the namespace anymore we can just use a nsPtrHashKey<nsIAtom> instead of CustomElementHashKey.

Comment 10

[Edgar Chen [:edgar]]

Thanks for your feedback. Will address your comment.

Comment 11

[Edgar Chen [:edgar]]

Attachment: Part 1: Move custom element codes from nsDocument to CustomElementsRegistry, v2

Comment 12

[Edgar Chen [:edgar]]

Attachment: Part 2: Implement CustomElementsRegistry define function, v2

Comment 13

[Edgar Chen [:edgar]]

Attachment: Part 1: Move custom element codes from nsDocument to CustomElementsRegistry, v3

Comment 14

[Edgar Chen [:edgar]]

Attachment: Part 2: Implement CustomElementsRegistry define function, v3

Address comment #9.

Comment 15

[Edgar Chen [:edgar]]

There is one more thing, we need to provide a way to lookup definition by constructor for
1). step 6 of https://html.spec.whatwg.org/multipage/scripting.html#concept-custom-element-definition-lifecycle-callbacks
and 2). step 2 of https://html.spec.whatwg.org/multipage/dom.html#htmlconstructor

Comment 16

[Edgar Chen [:edgar]]

(In reply to Edgar Chen [:edgar][:echen] from comment #15)
> There is one more thing, we need to provide a way to lookup definition by
> constructor for
> 1). step 6 of
> https://html.spec.whatwg.org/multipage/scripting.html#concept-custom-element-
> definition-lifecycle-callbacks
> and 2). step 2 of
> https://html.spec.whatwg.org/multipage/dom.html#htmlconstructor

Okay, I plan to support this in bug 1274159 or file a new bug if needed.

Comment 17

[Edgar Chen [:edgar]]

Comment on attachment 8780466 [details] [diff] [review]
Part 2: Implement CustomElementsRegistry define function, v3

Review of attachment 8780466 [details] [diff] [review]:
-----------------------------------------------------------------

Hi William,
With this patch, we can pass most of wpts for define [1], except some test cases for valid custom element names which will be handled in bug 1275832 and registering duplicated constructor (as I mentioned in comment #15 and comment #16).
Thank you.

[1] http://w3c-test.org/custom-elements/custom-elements-registry/define.html

Comment 18

[Edgar Chen [:edgar]]

dom/html/test/test_bug1081037.html failed in try run [1].

1287 INFO TEST-UNEXPECTED-FAIL | dom/html/test/test_bug1081037.html | non-configurable and not-writable constructor property (thrown:TypeError: can't redefine non-configurable property "constructor")
1290 INFO TEST-UNEXPECTED-FAIL | dom/html/test/test_bug1081037.html | number of getOwnPropertyDescriptor calls from registerElement: 0 - got +0, expected 1

Investigating ...

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=d58e35653c1ddc7595add1a9e22a766ced965315&filter-tier=1&group_state=expanded

Comment 19

[Edgar Chen [:edgar]]

Comment on attachment 8780466 [details] [diff] [review]
Part 2: Implement CustomElementsRegistry define function, v3

Review of attachment 8780466 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsDocument.cpp
@@ +5774,3 @@
>    JS::Rooted<JSObject*> protoObject(aCx);
>    {
> +    JSAutoCompartment ac(aCx, global);

Hmm, While I am investigating the failures in comment #18, I found this change causes HTMLElementProto doesn't be wrappered into caller's compartment.
So canceling the review request first. Will provide a new version to fix this and also comment #18.

Comment 20

[Edgar Chen [:edgar]]

I got another test failure in dom/base/test/chrome/test_registerElement_content.xul [1].
It seems because we didn't wrapper life-cycle callbacks into correct compartment (caller's compartment maybe). Investigating ...

[1] https://dxr.mozilla.org/mozilla-central/source/dom/base/test/chrome/test_registerElement_content.xul

Comment 21

[Edgar Chen [:edgar]]

Attachment: Part 2: Implement CustomElementsRegistry define function, v4

Fix test failures mentioned in comment #18 and comment #20.

Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a98690bdb9f911e92d222adf04d9763620840453&group_state=expanded&filter-tier=1

Comment 22

[Edgar Chen [:edgar]]

Attachment: Part 1: Move custom element codes from nsDocument to CustomElementsRegistry, v4

Sorry, I just realized that I uploaded a wrong part1 patch. Attach correct one.

Comment 23

[Edgar Chen [:edgar]]

Comment on attachment 8783536 [details] [diff] [review]
Part 2: Implement CustomElementsRegistry define function, v4

Review of attachment 8783536 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/CustomElementsRegistry.h
@@ +190,5 @@
> +  void UpgradeCandidates(JSContext* aCx,
> +                         nsIAtom* aKey,
> +                         CustomElementDefinition* aDefinition);
> +
> +  typedef nsClassHashtable<nsPtrHashKey<nsIAtom>, CustomElementDefinition>

nsIAtom is a reference-counted class, but nsPtrHashKey doesn't own a reference of nsIAtom. Hence, key may be released unexpectedly which cause intermittent failure in dom/tests/mochitest/webcomponents/test_document_register_lifecycle.html. We should better use nsISupportsHashKey.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=6412ff03b5b0c6060c7e3afad6a62cd4ad7d7f2a&group_state=expanded&filter-tier=1&selectedJob=26133979

Comment 24

[Edgar Chen [:edgar]]

Attachment: Part 2: Implement CustomElementsRegistry define function, v5

Addressed comment #23 and try result [1] looks perfect.

I apologize for the review request spam. This version should really be the final one. :p 

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d8e63ef0e396edb0bd6a72c65e1e9d47cf16471&group_state=expanded&filter-tier=1

Comment 25

[William Chen [:wchen]]

Comment on attachment 8783949 [details] [diff] [review]
Part 2: Implement CustomElementsRegistry define function, v5

Review of attachment 8783949 [details] [diff] [review]:
-----------------------------------------------------------------

Patch looks good, just have some comments and questions.

::: dom/base/CustomElementsRegistry.cpp
@@ +277,4 @@
>      return;
>    }
>  
> +  nsTArray<nsWeakPtr>* unresolved = mCandidatesMap.Get(typeName);

You should be able to use LookupOrAdd here.

@@ +466,5 @@
>  
> +void
> +CustomElementsRegistry::UpgradeCandidates(JSContext* aCx,
> +                                          nsIAtom* aKey,
> +                                          CustomElementDefinition* aDefinition)

The v0 upgrade is quite different from v1 upgrade, are you planning on updating the algorithm in another patch?

@@ +555,5 @@
> +        aRv.StealExceptionFromJSContext(cx); \
> +        return; \
> +      } \
> +    } \
> +  } \

Can you implement these helpers as static methods/functions instead of macros?

@@ +648,5 @@
> +  } // Leave constructorProtoUnwrapped's compartment.
> +
> +  /**
> +   * 3. If name is not a valid custom element name, then throw a "SyntaxError"
> +   *    DOMException and abort these steps.

The next few steps have moved up 1 spot.

@@ +669,5 @@
> +  /**
> +   * 6. If this CustomElementsRegistry contains an entry with constructor constructor,
> +   *    then throw a "NotSupportedError" DOMException and abort these steps.
> +   */
> +  // TODO: Bug 1274159 - Add [HTMLConstructor] to support custom element feature.

Why does this step depend on bug 1274159?

@@ +708,5 @@
> +      return;
> +    }
> +
> +    localName.Assign(aOptions.mExtends.Value());
> +  }

Step 2 in this patch moves here and you should also add the "element definition is running" flag.

@@ +782,5 @@
> +   */
> +  mCustomDefinitions.Put(nameAtom, definition);
> +
> +  // 16. 17. 18. Upgrade candidates
> +  UpgradeCandidates(cx, nameAtom, definition);

UpgradeCandidate will try to upgrade immediately, the spec says that elements should be upgraded in an element upgrade reaction so the timing will be a bit different here. Is this going to be done in another patch?

Comment 26

[Edgar Chen [:edgar]]

Comment on attachment 8783949 [details] [diff] [review]
Part 2: Implement CustomElementsRegistry define function, v5

Review of attachment 8783949 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/CustomElementsRegistry.cpp
@@ +277,4 @@
>      return;
>    }
>  
> +  nsTArray<nsWeakPtr>* unresolved = mCandidatesMap.Get(typeName);

Got it.

@@ +466,5 @@
>  
> +void
> +CustomElementsRegistry::UpgradeCandidates(JSContext* aCx,
> +                                          nsIAtom* aKey,
> +                                          CustomElementDefinition* aDefinition)

Yes, I am planning on handling v1 upgrade algorithm in a separated bug.
Will file the bug soon and add some comments with bug id.

@@ +555,5 @@
> +        aRv.StealExceptionFromJSContext(cx); \
> +        return; \
> +      } \
> +    } \
> +  } \

Will do.

@@ +648,5 @@
> +  } // Leave constructorProtoUnwrapped's compartment.
> +
> +  /**
> +   * 3. If name is not a valid custom element name, then throw a "SyntaxError"
> +   *    DOMException and abort these steps.

Ah, yes, will update the comments. 
And it is because the checks for HTMLElement at definition time was removed from spec in https://github.com/w3c/webcomponents/issues/541 few weeks ago. I think we should also remove them.

@@ +669,5 @@
> +  /**
> +   * 6. If this CustomElementsRegistry contains an entry with constructor constructor,
> +   *    then throw a "NotSupportedError" DOMException and abort these steps.
> +   */
> +  // TODO: Bug 1274159 - Add [HTMLConstructor] to support custom element feature.

Step 3 of HTMLConstructor [1] also needs a way to look up definition by using constructor. So I plans to figure out a solution to support both of them in bug 1274159 (maybe create another hash table which uses constructor as key).

[1] https://html.spec.whatwg.org/multipage/dom.html#htmlconstructor

@@ +708,5 @@
> +      return;
> +    }
> +
> +    localName.Assign(aOptions.mExtends.Value());
> +  }

Will add "element definition is running" flag.

@@ +782,5 @@
> +   */
> +  mCustomDefinitions.Put(nameAtom, definition);
> +
> +  // 16. 17. 18. Upgrade candidates
> +  UpgradeCandidates(cx, nameAtom, definition);

Yes, I am planing on implementing v1 upgrade in another bug.

Comment 27

[Edgar Chen [:edgar]]

(In reply to Edgar Chen [:edgar][:echen] from comment #26)
> Comment on attachment 8783949 [details] [diff] [review]
> Part 2: Implement CustomElementsRegistry define function, v5
> 
> Review of attachment 8783949 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/CustomElementsRegistry.cpp
> 
> @@ +466,5 @@
> >  
> > +void
> > +CustomElementsRegistry::UpgradeCandidates(JSContext* aCx,
> > +                                          nsIAtom* aKey,
> > +                                          CustomElementDefinition* aDefinition)
> 
> Yes, I am planning on handling v1 upgrade algorithm in a separated bug.
> Will file the bug soon and add some comments with bug id.
> 
> @@ +782,5 @@
> > +   */
> > +  mCustomDefinitions.Put(nameAtom, definition);
> > +
> > +  // 16. 17. 18. Upgrade candidates
> > +  UpgradeCandidates(cx, nameAtom, definition);
> 
> Yes, I am planing on implementing v1 upgrade in another bug.

Filed bug 1299363 for v1 upgrade.

Comment 28

[Edgar Chen [:edgar]]

Attachment: Part 1: Move custom element codes from nsDocument to CustomElementsRegistry, v5, r=wchen

Rebase only.

Comment 29

[Edgar Chen [:edgar]]

Attachment: Part 2: Implement CustomElementsRegistry define function, v6

Comment 30

[Edgar Chen [:edgar]]

Attachment: interdiff_part2_v5_v6

Comment 31

[Edgar Chen [:edgar]]

Comment on attachment 8786683 [details] [diff] [review]
Part 2: Implement CustomElementsRegistry define function, v6

Review of attachment 8786683 [details] [diff] [review]:
-----------------------------------------------------------------

Address review comment #25
- Use LookupOrAdd in RegisterUnresolv().
- Implement helpers as static function.
- Implement "element definition is running" flag.
- Sync comment with latest spec.
- Remove prototype checks in Define() per spec change [1] and add the protoptype check in RegisterElement() back.
  (Corresponding wpt tests are removed from upstream [2], but the change isn't merged to our codebase yet.)  

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9ed15a7eda7276296a3cf60d0a54d96a57e0536&filter-tier=1&group_state=expanded 

[1] https://github.com/w3c/webcomponents/issues/541
[2] https://github.com/w3c/web-platform-tests/commit/a64b7d648bf218d7882461352dad6fce99df6ac5

Comment 32

[William Chen [:wchen]]

Comment on attachment 8786683 [details] [diff] [review]
Part 2: Implement CustomElementsRegistry define function, v6

Review of attachment 8786683 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, but I'd like to see the updated patch before r+

::: dom/base/CustomElementsRegistry.cpp
@@ +229,5 @@
>  CustomElementsRegistry::sProcessingStack;
>  
>  CustomElementsRegistry::CustomElementsRegistry(nsPIDOMWindowInner* aWindow)
>   : mWindow(aWindow)
> + , IsCustomDefinitionRunning(false)

mIsCustomDefinitionRunning

@@ +528,5 @@
> +  "detachedCallback"
> +};
> +
> +static bool
> +CheckLifeCycleCallbacks(JSContext* aCx,

Instead of returning a success bool, the callers can check if aRv failed. When you implement the callbacks, you will probably want to return the callback from this function to store in the definition.

@@ +746,5 @@
> +        aRv.Throw(NS_ERROR_FAILURE);
> +        break;
> +      }
> +    } // Leave constructorProtoUnwrapped's compartment.
> +  } while(false);

Instead of using this pattern, make a small RAII class to handle setting/unsetting the "is custom element definition running" flag and return whenever aRv failed.

Comment 33

[Edgar Chen [:edgar]]

Comment on attachment 8786683 [details] [diff] [review]
Part 2: Implement CustomElementsRegistry define function, v6

Review of attachment 8786683 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/CustomElementsRegistry.cpp
@@ +746,5 @@
> +        aRv.Throw(NS_ERROR_FAILURE);
> +        break;
> +      }
> +    } // Leave constructorProtoUnwrapped's compartment.
> +  } while(false);

Oh, yes, using RAII pattern is a much better way. Will do.

Comment 34

[Edgar Chen [:edgar]]

Attachment: Part 2: Implement CustomElementsRegistry define function, v7

Comment 35

[Edgar Chen [:edgar]]

Attachment: interdiff_part2_v6_v7

Comment 36

[Edgar Chen [:edgar]]

Comment on attachment 8787094 [details] [diff] [review]
Part 2: Implement CustomElementsRegistry define function, v7

Review of attachment 8787094 [details] [diff] [review]:
-----------------------------------------------------------------

Address review comment #32
- s/IsCustomDefinitionRunning/mIsCustomDefinitionRunning/.
- CheckLifeCycleCallbacks() don't need to return a success bool.
- make a small RAII class to handle setting/unsetting the "is custom element definition running" flag.

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ae770e1d30dfb10c0cc3708472d4463676f67f4&filter-tier=1&group_state=expanded

Comment 37

[William Chen [:wchen]]

Comment on attachment 8787094 [details] [diff] [review]
Part 2: Implement CustomElementsRegistry define function, v7

Review of attachment 8787094 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/CustomElementsRegistry.h
@@ +219,5 @@
> +  // It is used to prevent reentrant invocations of element definition.
> +  bool mIsCustomDefinitionRunning;
> +
> +private:
> +  class FlagMarker {

Add MOZ_RAII and final annotation to the class. Please also rename the class to AutoSomething to make it more obvious that this is an RAII class, maybe AutoSetRunningFlag. This class can also be simpler, you can just hold a raw pointer to the CustomElementRegistry and set the flag to true in the constructor and false in the destructor. Also add an assertion in the constructor that the flag is initially false.

Comment 38

[Edgar Chen [:edgar]]

Attachment: Part 2: Implement CustomElementsRegistry define function, v8, r=wchen

Address comment #37. Thanks for the review, William.

Comment 39

[Edgar Chen [:edgar]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e49bccec4eede7c9004a3335ce01c65cb81e7d5d&filter-tier=1&group_state=expanded

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f2f37ec0fd3
Part 1: Move custom element codes from nsDocument to CustomElementsRegistry; r=wchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/9cc407277fd0
Part 2: Implement CustomElementsRegistry define function; r=wchen

Comment 41

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1f2f37ec0fd3
https://hg.mozilla.org/mozilla-central/rev/9cc407277fd0

Comment 42

[Joel Maher ( :jmaher ) (UTC -8)]

this increased the num_constructors:
== Change summary for alert #2944 (as of September 02 2016 02:52 UTC) ==

Regressions:

  1%  compiler_metrics num_constructors linux64 pgo       170 -> 171
  1%  compiler_metrics num_constructors linux32 opt       170 -> 171
  0%  compiler_metrics num_constructors linux32 debug     231 -> 232
  0%  compiler_metrics num_constructors linux64 debug     231 -> 232

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=2944

:froydnj, can you make sure this is proper?

Comment 43

[Nathan Froyd [:froydnj]]

(In reply to Joel Maher ( :jmaher) from comment #42)
> this increased the num_constructors:
> == Change summary for alert #2944 (as of September 02 2016 02:52 UTC) ==
> 
> Regressions:
> 
>   1%  compiler_metrics num_constructors linux64 pgo       170 -> 171
>   1%  compiler_metrics num_constructors linux32 opt       170 -> 171
>   0%  compiler_metrics num_constructors linux32 debug     231 -> 232
>   0%  compiler_metrics num_constructors linux64 debug     231 -> 232
> 
> For up to date results, see:
> https://treeherder.mozilla.org/perf.html#/alerts?id=2944
> 
> :froydnj, can you make sure this is proper?

I think this is an unfortunate side-effect of moving nsDocument::sProcessingStack to CustomElementsRegistry::sProcessingStack.  Due to unified compilation, whatever unified subset of files CustomElementsRegistry.cpp belongs to didn't have a static constructor, whereas whatever unified subset of files nsDocument.cpp belongs to already had a static constructor (from sProcessingStack and whatever else lives in those unified files).  Moving sProcessingStack into CustomElementsRegistry.cpp, then, caused a static constructor to be created for that subset of unified files, while not eliminating the one in nsDocument.cpp's subset.  So there's not any change in the amount of code we're running, merely where it's living.