1315885 - Implement callback reaction for custom element reactions

Status: RESOLVED FIXED

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

Description

[John Dai[:jdai]]

According to spec [1], we need to implement callback reaction for custom element reactions. Also, enqueue a custom element callback reaction algorithm and invoke custom element reactions algorithm.

[1] https://html.spec.whatwg.org/multipage/scripting.html#callback-reaction

Comment 1

[John Dai[:jdai]]

Hi William,
When I was implementing this feature, I found there is no more createdCallback in new specification.[1]
In the old specification[2] said, "If the created callback exists for an element, all other callbacks must not be enqueued until after this created callback's invocation had started."[2] Hence, we have a test case[3] which tests this part. However, in the new specification setAttribute has a [CEReactions] tag which means it is a synchronously call. Therefore, it can't be enqueued until after this created callback's invocation had started.
I am not sure should I need to support this v0 feature or I can modify our test case to not support this v0 feature? As I know we'll abandon v0 code in someday. Is there any good suggestion to deal with this issue? Thank you.


[1] new specification: https://html.spec.whatwg.org/multipage/scripting.html#custom-element-reactions 
[2] old specification: https://www.w3.org/TR/2016/WD-custom-elements-20160226/#dfn-created-callback 
[3] https://searchfox.org/mozilla-central/source/dom/tests/mochitest/webcomponents/test_document_register_stack.html#30-31

Comment 2

[William Chen [:wchen]]

We shouldn't let v0 inhibit the new implementation. Let's relax the [2] condition and update the test. Even when B2G was using the v0 implementation, I don't think I ever saw anything depend on [2].

Comment 3

[John Dai[:jdai]]

Attachment: wip, v1

- Implement custom element v1 callback reaction, and integrate it with custom element v0 
  in order to pass our test cases.
- Introduce CustomElementCallbackReaction which stores CustomElementCallback. 
- Remove v0 data properties and data member in CustomElementData, such as mCallbackQueue,
  mCurrentCallback, mAssociatedMicroTask, RunCallbackQueue() and sProcessingStack.
- Comment out test_document_register_stack.html because we won't let v0 inhibit 
  the new implementation. 
- Add SuppressException in CustomElementCallback::Call because we have been handled error
  in CallSetup.[1]
- Add [CEReactions] tags in webidl for testing purpose. 
- Introduce InvokeCallbackReaction() for SetupCustomElement() which needs to call 
  reaction synchronously.

This patch need to rebase after both bug 1309147 and bug 1325279 are landed.

[1] https://searchfox.org/mozilla-central/source/dom/bindings/CallbackObject.cpp#192

Comment 4

[John Dai[:jdai]]

Attachment: wip, v2

- Revised InvokeReactions() which transfers the ownership of the reaction due to reentrant invocation of InvokeReactions().
- Introduce SyncInvokeReactions() synchronously for createElement/createElementNS in order to pass tests.

Comment 5

[John Dai[:jdai]]

Attachment: patch, v1

- Revise nsDOMSlots::Traverse which traverses ReactionQueue's reactions.
- Use UniquePtr to manage resource for SyncInvokeReactions().

Comment 6

[Edgar Chen [:edgar]]

Comment on attachment 8884181 [details] [diff] [review]
patch, v1

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

Overall, it looks good, but I'd like another look. Thank you!

::: dom/base/CustomElementRegistry.cpp
@@ +57,5 @@
>    }
> +
> +  // If callbacks throw exceptions, it'll be handled and reported in
> +  // LifecycleCreatedCallback::Call function.
> +  rv.SuppressException();

This probably could be put in a separated patch.

@@ +290,5 @@
>  
>    // Enqueuing the created callback will set the CustomElementData on the
>    // element, causing prototype swizzling to occur in Element::WrapObject.
> +  // We make it synchronously for createElement/createElementNS in order to
> +  // pass tests. It'll be removed when bug 1301024 is landed.

It is not quit clear to me why removing this behavior would be related to bug 1301024.

@@ +296,4 @@
>  }
>  
> +CustomElementCallback*
> +CustomElementRegistry::CreateCustomElementCallback(

Make this function returns a UniquePtr<CustomElementCallback>, so that we could force the caller to use UniquePtr to avoid possible memory leak.

@@ +970,5 @@
>      nsTArray<nsAutoPtr<CustomElementReaction>>& reactions =
>        elementData->mReactionQueue;
>      for (uint32_t j = 0; j < reactions.Length(); ++j) {
> +      // Transfer the ownership of the reaction due to reentrant invocation of
> +      // this funciton.

These changes probably could be put in a separated part.

@@ +971,5 @@
>        elementData->mReactionQueue;
>      for (uint32_t j = 0; j < reactions.Length(); ++j) {
> +      // Transfer the ownership of the reaction due to reentrant invocation of
> +      // this funciton.
> +      nsAutoPtr<CustomElementReaction> reaction = reactions.ElementAt(j);

Use UniquePtr.

@@ +972,5 @@
>      for (uint32_t j = 0; j < reactions.Length(); ++j) {
> +      // Transfer the ownership of the reaction due to reentrant invocation of
> +      // this funciton.
> +      nsAutoPtr<CustomElementReaction> reaction = reactions.ElementAt(j);
> +      if (reaction) {

The ownership transferring leaves null entry in reaction queue, so we need to add null checks whenever we access the entry of reaction queue which is weird to me. I think we should remove the entry, instead of leaving a null entry in the reaction queue, but it could be addressed in a follow up bug. Please file a follow up bug and add the bug number in the comment.

::: dom/base/CustomElementRegistry.h
@@ +184,5 @@
> +    explicit CustomElementCallbackReaction(CustomElementRegistry* aRegistry,
> +                                           CustomElementDefinition* aDefinition,
> +                                           CustomElementCallback* aCustomElementCallback)
> +      : CustomElementReaction(aRegistry, aDefinition),
> +        mCustomElementCallback(aCustomElementCallback)

: CustomElementReaction(aRegistry, aDefinition)
, mCustomElementCallback(aCustomElementCallback)

@@ +195,5 @@
> +    }
> +
> +  private:
> +    virtual void Invoke(Element* aElement) override;
> +    nsAutoPtr<CustomElementCallback> mCustomElementCallback;

Use UniquePtr.

::: dom/tests/mochitest/webcomponents/test_document_register_stack.html
@@ +27,5 @@
>      is(createdCallbackCalled, false, "Created callback should be called before attached callback.");
>      createdCallbackCalled = true;
>      is(attributeChangedCallbackCalled, false, "Attribute changed callback should not have been called prior to setting the attribute.");
>      this.setAttribute("foo", "bar");
> +    // is(attributeChangedCallbackCalled, false, "While element is being created, element should not be added to the current element callback queue.");

Change the condition to true and update the explanation, instead of remove it.

::: dom/webidl/ShadowRoot.webidl
@@ +16,5 @@
>    Element? getElementById(DOMString elementId);
>    HTMLCollection getElementsByTagName(DOMString localName);
>    HTMLCollection getElementsByTagNameNS(DOMString? namespace, DOMString localName);
>    HTMLCollection getElementsByClassName(DOMString classNames);
> +  [CEReactions, SetterThrows, TreatNullAs=EmptyString]

Put this change in a separated patch.

Comment 7

[John Dai[:jdai]]

Attachment: Bug 1315885 - Part 1: Add ShadowRoot CEReactions annotation.

Comment 8

[John Dai[:jdai]]

Attachment: Bug 1315885 - Part 2: Avoid rethrowing exception in CustomElementCallback::Call.

Comment 9

[John Dai[:jdai]]

Attachment: Bug 1315885 - Part 3: Transfer the ownership of ReactionQueue's entry due to re-enter CustomElementReactionsStack::InvokeReactions.

> @@ +972,5 @@
> >      for (uint32_t j = 0; j < reactions.Length(); ++j) {
> > +      // Transfer the ownership of the reaction due to reentrant invocation of
> > +      // this funciton.
> > +      nsAutoPtr<CustomElementReaction> reaction = reactions.ElementAt(j);
> > +      if (reaction) {
> 
> The ownership transferring leaves null entry in reaction queue, so we need
> to add null checks whenever we access the entry of reaction queue which is
> weird to me. I think we should remove the entry, instead of leaving a null
> entry in the reaction queue, but it could be addressed in a follow up bug.
> Please file a follow up bug and add the bug number in the comment.
File bug 1379573.

Comment 10

[John Dai[:jdai]]

Attachment: Bug 1315885 - Part 4: Implement callback reaction for custom element reactions.

Comment 11

[John Dai[:jdai]]

Comment on attachment 8884762 [details] [diff] [review]
Bug 1315885 - Part 4: Implement callback reaction for custom element reactions.

Cancel the review request, it's because I use lambda capture initializers which only available with C++14 and it causes try build error.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f74869136b43b69e34843a6f2b826aac6c59e78

Comment 12

[Edgar Chen [:edgar]]

Comment on attachment 8884760 [details] [diff] [review]
Bug 1315885 - Part 2: Avoid rethrowing exception in CustomElementCallback::Call.

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

::: dom/base/CustomElementRegistry.cpp
@@ +56,5 @@
>        break;
>    }
> +
> +  // If callbacks throw exceptions, it'll be handled and reported in
> +  // LifecycleCreatedCallback::Call function.

Not only LifecycleCreatedCallback would handle the exception, but all Lifecycle*Callback would do the same thing. So s/LifecycleCreatedCallback/Lifecycle*Callback/

Comment 13

[Edgar Chen [:edgar]]

Comment on attachment 8884761 [details] [diff] [review]
Bug 1315885 - Part 3: Transfer the ownership of ReactionQueue's entry due to re-enter CustomElementReactionsStack::InvokeReactions.

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

::: dom/base/CustomElementRegistry.cpp
@@ +998,5 @@
>  
>      RefPtr<CustomElementData> elementData = element->GetCustomElementData();
>      MOZ_ASSERT(elementData, "CustomElementData should exist");
>  
> +    auto& reactions = elementData->mReactionQueue;

Please remove this change if it is not necessary.

@@ +1003,3 @@
>      for (uint32_t j = 0; j < reactions.Length(); ++j) {
> +      // Transfer the ownership of the entry due to reentrant invocation of
> +      // this funciton. The entry will be removed when bug 1379573 is landed.

It is worth adding a test case for this.

Comment 14

[John Dai[:jdai]]

Attachment: Bug 1315885 - Part 2: Avoid rethrowing exception in CustomElementCallback::Call.

Address comment #12 and carry f+.

Comment 15

[John Dai[:jdai]]

Attachment: Bug 1315885 - Part 2: Avoid rethrowing exception in CustomElementCallback::Call.

Upload the correct patch.

Comment 16

[John Dai[:jdai]]

Attachment: Bug 1315885 - Part 4: Implement callback reaction for custom element reactions.

Since we can't use lambda capture initializers which only available with C++14, I use raw pointer and handle resource lifecycle by myself.

Comment 17

[Edgar Chen [:edgar]]

Comment on attachment 8885195 [details] [diff] [review]
Bug 1315885 - Part 4: Implement callback reaction for custom element reactions.

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

::: dom/base/CustomElementRegistry.cpp
@@ +377,5 @@
> +CustomElementRegistry::SyncInvokeReactions(nsIDocument::ElementCallbackType aType,
> +                                           Element* aCustomElement,
> +                                           CustomElementDefinition* aDefinition)
> +{
> +

Nit: remove this blank line.

@@ +380,5 @@
> +{
> +
> +  auto callback = CreateCustomElementCallback(aType, aCustomElement, nullptr,
> +                                              aDefinition);
> +

Nit: remove this blank line.

@@ +392,5 @@
> +  nsCOMPtr<nsIRunnable> runnable =
> +    NS_NewRunnableFunction("CustomElementRegistry::SyncInvokeReactions",
> +                           [reaction, aCustomElement]() {
> +      reaction->Invoke(aCustomElement);
> +      delete reaction;

Instead of using this pattern (manually delete), you could make a small helper class inheriting from mozilla::Runnable and use UniquePtr to manage the life-cycle of CustomElementReaction.

@@ +405,5 @@
> +                                                CustomElementDefinition* aDefinition)
> +{
> +  auto callback =
> +    CreateCustomElementCallback(aType, aCustomElement, aArgs, aDefinition);
> +

Nit: remove this blank line.

::: dom/base/CustomElementRegistry.h
@@ +180,5 @@
>  
> +class CustomElementCallbackReaction final : public CustomElementReaction
> +{
> +  public:
> +    explicit CustomElementCallbackReaction(CustomElementRegistry* aRegistry,

Don't need explicit since the constructor has move than one argument.

@@ +188,5 @@
> +      , mCustomElementCallback(Move(aCustomElementCallback))
> +    {
> +    }
> +
> +    void Traverse(nsCycleCollectionTraversalCallback& aCb) const override

virtual void Traverse

Comment 18

[John Dai[:jdai]]

Attachment: Bug 1315885 - Part 4: Implement callback reaction for custom element reactions.

Address comment #17.

Comment 19

[Edgar Chen [:edgar]]

Comment on attachment 8886017 [details] [diff] [review]
Bug 1315885 - Part 4: Implement callback reaction for custom element reactions.

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

::: dom/base/CustomElementRegistry.h
@@ +182,5 @@
> +{
> +  public:
> +    CustomElementCallbackReaction(CustomElementRegistry* aRegistry,
> +                                           CustomElementDefinition* aDefinition,
> +                                           UniquePtr<CustomElementCallback> aCustomElementCallback)

Nit: align arguments.

@@ +407,5 @@
>      private:
>        CustomElementRegistry* mRegistry;
>    };
>  
> +  class SyncInvokeReactionsRunnable : public mozilla::Runnable {

This help class will only invoke one reaction, so
s/SyncInvokeReactionsRunnable/SyncInvokeReactionRunnable/

@@ +409,5 @@
>    };
>  
> +  class SyncInvokeReactionsRunnable : public mozilla::Runnable {
> +    public:
> +      explicit SyncInvokeReactionsRunnable(

Don't need explicit since this constructor has more than on argument.

@@ +413,5 @@
> +      explicit SyncInvokeReactionsRunnable(
> +        UniquePtr<CustomElementReaction> aReaction, Element* aCustomElement)
> +        : Runnable(
> +            "dom::CustomElementRegistry::SyncInvokeReactionsRunnable")
> +        , mReaction(Move(aReaction)), mCustomElement(aCustomElement)

, mReaction(Move(aReaction))
, mCustomElement(aCustomElement)

Comment 20

[John Dai[:jdai]]

Attachment: Bug 1315885 - Part 4: Implement callback reaction for custom element reactions.

Address comment #19, and carry f+.

Comment 21

[John Dai[:jdai]]

Attachment: Bug 1315885 - Part 4: Implement callback reaction for custom element reactions.

Upload the correct patch.

Comment 22

[John Dai[:jdai]]

(In reply to Edgar Chen [:edgar] (parental leave ~ 10/1; slow response) from comment #13)
> Comment on attachment 8884761 [details] [diff] [review]
> Bug 1315885 - Part 3: Transfer the ownership of ReactionQueue's entry due to
> re-enter CustomElementReactionsStack::InvokeReactions.
> 
> Review of attachment 8884761 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/CustomElementRegistry.cpp
> @@ +998,5 @@
> >  
> >      RefPtr<CustomElementData> elementData = element->GetCustomElementData();
> >      MOZ_ASSERT(elementData, "CustomElementData should exist");
> >  
> > +    auto& reactions = elementData->mReactionQueue;
> 
> Please remove this change if it is not necessary.
> 
This change came from |nsTArray<nsAutoPtr<CustomElementReaction>>& reactions = elementData->mReactionQueue;| and I changed nsAutoPtr to UniquePtr. Hence, I guess this change is necessary.

> @@ +1003,3 @@
> >      for (uint32_t j = 0; j < reactions.Length(); ++j) {
> > +      // Transfer the ownership of the entry due to reentrant invocation of
> > +      // this funciton. The entry will be removed when bug 1379573 is landed.
> 
> It is worth adding a test case for this.
We have a test case which is test_document_register_stack.html will test this portion.

Comment 23

[William Chen [:wchen]]

Comment on attachment 8884761 [details] [diff] [review]
Bug 1315885 - Part 3: Transfer the ownership of ReactionQueue's entry due to re-enter CustomElementReactionsStack::InvokeReactions.

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

::: dom/base/CustomElementRegistry.cpp
@@ +1003,3 @@
>      for (uint32_t j = 0; j < reactions.Length(); ++j) {
> +      // Transfer the ownership of the entry due to reentrant invocation of
> +      // this funciton. The entry will be removed when bug 1379573 is landed.

I don't think we need to do what is in bug 137957.

Comment 24

[John Dai[:jdai]]

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

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8884758 [details] [diff] [review]
Bug 1315885 - Part 1: Add ShadowRoot CEReactions annotation.

I guess. This is about v0 API.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d592ae3b61bf
Part 1: Add ShadowRoot CEReactions annotation. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ba49d85f30c
Part 2: Avoid rethrowing exception in CustomElementCallback::Call. r=wchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8bc78492b81
Part 3: Transfer the ownership of ReactionQueue's entry due to re-enter CustomElementReactionsStack::InvokeReactions. r=wchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6dcdfe7ac54
Part 4: Implement callback reaction for custom element reactions. r=wchen

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Backed out for Part 4 merge conflicts with bug 1377993 :(

https://hg.mozilla.org/integration/mozilla-inbound/rev/944c576e7e20

Comment 28

[John Dai[:jdai]]

Attachment: Bug 1315885 - Part 4: Implement callback reaction for custom element reactions.

Rebase and carry r+.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4120a7d9f16c
Part 1: Add ShadowRoot CEReactions annotation. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/60f70ae7c82d
Part 2: Avoid rethrowing exception in CustomElementCallback::Call. r=wchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/7366a7028d98
Part 3: Transfer the ownership of ReactionQueue's entry due to re-enter CustomElementReactionsStack::InvokeReactions. r=wchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/62294b20e47f
Part 4: Implement callback reaction for custom element reactions. r=wchen

Comment 30

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

https://hg.mozilla.org/mozilla-central/rev/4120a7d9f16c
https://hg.mozilla.org/mozilla-central/rev/60f70ae7c82d
https://hg.mozilla.org/mozilla-central/rev/7366a7028d98
https://hg.mozilla.org/mozilla-central/rev/62294b20e47f