Closed Bug 1460815 Opened 6 years ago Closed 6 years ago

Provide a chrome-only callback on CustomElementRegistry so script can define CE lazily

Categories

(Core :: DOM: Core & HTML, task, P5)

Product:
Core
Component:
DOM: Core & HTML
Version:
59 Branch
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: timdream, Assigned: timdream)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1460815 - Provide a chrome-only callback on CustomElementRegistry so script can define CE lazily
59 bytes, text/x-review-board-request
smaug
: review+
Details 
In bug 1411707 it was found that moving > 1K LOC of XBL binding code to XUL document start-up has a performance impact.

We will need a way to define these "global" CEs in MainProcessSingleton.js without having such performance impact.

Without any help from the platform, a JavaScript-only solution would be to define a placeholder class and populate its properties from the script on construction. This sadly would prevent us from defining a CE with a plain JS object as its not possible to set static properties after-the-fact.

I have a WIP ready that executes the callback passed when the name matches, and allow us to call customElements.define() right before the CE construction.

Note on <findbar>: it is true that [1] in tabbrowser.js (and test scripts) are the only places that use it, and we can certainly load findbar.js there. It is however not a generic solution that could help overall XBL removal progress.
Tests look good.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ceb98c969cc9dcf4f3c2235202b6d5e3bf138221

Brian will comment on the Talos result of this patch alone.

Naming is hard ... the name I ended up settling with is customElements.setElementCreationCallback(), please comment if you have better names.
Comment on attachment 8975104 [details]
Bug 1460815 - Provide a chrome-only callback on CustomElementRegistry so script can define CE lazily

https://reviewboard.mozilla.org/r/243478/#review249674

::: dom/tests/mochitest/webcomponents/test_custom_element_set_element_creation_callback.html:7
(Diff revision 2)
> +<html>
> +<!--
> +https://bugzilla.mozilla.org/show_bug.cgi?id=1460815
> +-->
> +<head>
> +  <title>Test for customElements.define</title>

Update this.
(In reply to Brian Grinstead [:bgrins] from comment #5)
> Talos comparison (ongoing):
> https://treeherder.mozilla.org/perf.html#/
> compare?originalProject=try&originalRevision=339a5b978829af9f9337a6311d5b6b11
> 496c8131&newProject=try&newRevision=66d20e7fb2876c0b089d476d7e9aedee9f66028b&
> framework=1&showOnlyImportant=1

I see a linux64 regression on `a11yr opt e10s stylo`, but I'm inclined to think it's noise since it's linux only and contained within that test (I would have expected this to hit many different tests if it was actually having an effect).  Probably worth getting review at this point and we can do another run after any changes.
Attachment #8975104 - Flags: review?(bugs)
Comment on attachment 8975104 [details]
Bug 1460815 - Provide a chrome-only callback on CustomElementRegistry so script can define CE lazily

https://reviewboard.mozilla.org/r/243478/#review249938

::: dom/base/CustomElementRegistry.h:441
(Diff revision 2)
>    DefinitionMap mCustomDefinitions;
>  
> +  // Hashtable for chrome-only callbacks that is called *before* we return
> +  // a CustomElementDefinition, when the typeAtom matches.
> +  // The callbacks are registered with the setElementCreationCallback method.
> +  mutable ElementCreationCallbackMap mElementCreationCallbacks;

A tad ugly to use mutable member variable. Just make
LookupCustomElementDefinition non-const.

::: dom/base/CustomElementRegistry.cpp:312
(Diff revision 2)
>  CustomElementRegistry::LookupCustomElementDefinition(nsAtom* aNameAtom,
>                                                       nsAtom* aTypeAtom) const
>  {
> +  {
> +    RefPtr<CustomElementCreationCallback> callback;
> +    mElementCreationCallbacks.Get(aTypeAtom, getter_AddRefs(callback));

I think accessing mElementCreationCallbacks should be done after trying get definition from mCustomDefinitions, since in normal case the latter should return some sane value rather often.
So, first try to get value from mCustomDefinitions, if null, then mElementCreationCallbacks, and if there was a callback, try to then access mCustomDefinitions again.

::: dom/base/CustomElementRegistry.cpp:318
(Diff revision 2)
> +    if (callback) {
> +      MOZ_ASSERT(!mCustomDefinitions.GetWeak(aTypeAtom), "Callback should not exist if definition is loaded.");
> +
> +      ErrorResult er;
> +      nsString value;
> +      aTypeAtom->ToString(value);

nsDependentAtomString

::: dom/base/CustomElementRegistry.cpp:319
(Diff revision 2)
> +      MOZ_ASSERT(!mCustomDefinitions.GetWeak(aTypeAtom), "Callback should not exist if definition is loaded.");
> +
> +      ErrorResult er;
> +      nsString value;
> +      aTypeAtom->ToString(value);
> +      callback->Call(value, er);

So if the Call fails, er will MOZ_ASSERT that it has unhandled exception.
You may want to use IgnoredErrorResult

::: dom/base/CustomElementRegistry.cpp:975
(Diff revision 2)
>    }
>  
> +  /**
> +   * Clean-up mElementCreationCallbacks (if it exists)
> +   */
> +  mElementCreationCallbacks.Put(nameAtom, nullptr);

don't replace with null, but just remove the entry.
Attachment #8975104 - Flags: review?(bugs) → review-
Comment on attachment 8975104 [details]
Bug 1460815 - Provide a chrome-only callback on CustomElementRegistry so script can define CE lazily

https://reviewboard.mozilla.org/r/243478/#review249938

> So if the Call fails, er will MOZ_ASSERT that it has unhandled exception.
> You may want to use IgnoredErrorResult

I don't want to silent unhandled exception here; Having MOZ_ASSERT kicks in seems fine?
that MOZ_ASSERT in ErrorResult is there to tell someone is misusing ErrorResult API.
If you want to assert about the exception, do it explicitly in the code using ErrorResult
Comment on attachment 8975104 [details]
Bug 1460815 - Provide a chrome-only callback on CustomElementRegistry so script can define CE lazily

https://reviewboard.mozilla.org/r/243478/#review250136

::: dom/base/CustomElementRegistry.cpp:320
(Diff revision 3)
> +    if (callback) {
> +      MOZ_ASSERT(!mCustomDefinitions.GetWeak(aTypeAtom), "Callback should not exist if definition is loaded.");
> +
> +      ErrorResult er;
> +      nsDependentAtomString value(aTypeAtom);
> +      callback->Call(value, er);

Sorry, I wasn't thinking this before, but I don't think calling JS at this point is really safe. The C++ callers really don't expect random JS to run here. We need to use a script runner or somehow push the call to happen when custom element reaction queue.

::: dom/base/CustomElementRegistry.cpp:321
(Diff revision 3)
> +      MOZ_ASSERT(!mCustomDefinitions.GetWeak(aTypeAtom), "Callback should not exist if definition is loaded.");
> +
> +      ErrorResult er;
> +      nsDependentAtomString value(aTypeAtom);
> +      callback->Call(value, er);
> +

So you need to take the exception from er, and explicitly assert about it. We don't want to misuse ErrorResult API.
So, call StealNSResult() or some such and do something like
MOZ_ASSERT(NS_SUCCEEDED(er.StealNSResult()));
Attachment #8975104 - Flags: review?(bugs) → review-
The latest patch added the script runner. The first element gets created in JS will now have to go through the upgrade route. Please check the interdiff on the test file to find out.
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #15)
> The latest patch added the script runner. The first element gets created in
> JS will now have to go through the upgrade route. Please check the interdiff
> on the test file to find out.

This is actually pretty sad, to be honest. I have not test what would happen for elements constructed by the parser, but this can be an evil trap for element created from JS.

Specifically for JS, I think we can workaround it by wrapping |createElement()| and |createElementNS()| on every document. Do we want to do that? Is it too much?
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #16)
> (In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment
> #15)
> > The latest patch added the script runner. The first element gets created in
> > JS will now have to go through the upgrade route. Please check the interdiff
> > on the test file to find out.
> 
> This is actually pretty sad, to be honest. I have not test what would happen
> for elements constructed by the parser, but this can be an evil trap for
> element created from JS.
> 
> Specifically for JS, I think we can workaround it by wrapping
> |createElement()| and |createElementNS()| on every document. Do we want to
> do that? Is it too much?

Yeah, I don't like having to go through the upgrade path since it will lead to inconsistencies like we see with XBL. Do we know when exactly the script will execute? I'm assuming it will lead to a very similar situation that we have in XBL (as outlined in https://bugzilla.mozilla.org/show_bug.cgi?id=1411707#c96).

```
var myEl = document.createElement("my-element");
myEl.val = 1;
document.body.appendChild(myEl); // shows "connected undefined"

customElements.define("my-element", class Foo extends HTMLElement {
  connectedCallback() { this.textContent = "connected " + this._val; }
  set val(value) { this._val = value; }
});

// This could happen later on, and won't call the setter since
// `val` was set as an expando property before the CE was upgraded.
myEl.val = 2;
```

as opposed to the non-upgrade path which is:

```
customElements.define("my-element", class Foo extends HTMLElement {
  connectedCallback() { this.textContent = "connected " + this._val; }
  set val(value) { this._val = value; }
});

var myEl = document.createElement("my-element");
myEl.val = 1;
document.body.appendChild(myEl); // shows "connected 1"
```
I guess if we knew exactly when the script would run (i.e. right before or after the appendChild) it'd be better than XBL at least, since (a) it'd be more predictable than having to wait for layout and (b) we wouldn't be stuck in a broken state where the element never gets the CE applied, so if we were careful to not touch any properties until after appending things would still work.
So I was expecting upgrade to happen when the callback has run, assuming the callback has added a new CE definition.
So RunCustomElementCreationCallback::Run would force upgrade on the elements named mAtom
Comment on attachment 8975104 [details]
Bug 1460815 - Provide a chrome-only callback on CustomElementRegistry so script can define CE lazily

https://reviewboard.mozilla.org/r/243478/#review250230

Sounds like we want a bit different behavior
Attachment #8975104 - Flags: review?(bugs)
(In reply to Olli Pettay [:smaug] from comment #19)
> So I was expecting upgrade to happen when the callback has run, assuming the
> callback has added a new CE definition.
> So RunCustomElementCreationCallback::Run would force upgrade on the elements
> named mAtom

That's indeed what would happen, it just that previously callback runs *before* LookupCustomElementDefinition() returns, therefore even the first element creation gets the definition (therefore, created as a custom element without the need to upgrade).
After looking more closely at the test case and Tim and I discussing in more detail I think the solution here is fine. I'd prefer if we upgraded the first element immediately without having to wait for it to be appended into the DOM to prevent potential issues like in Comment 17, but that behavior is better than what we have today with XBL. And since XBL has similar issues, existing callers in tree shouldn't be relying on attachment before/after being appended into the DOM.
Comment on attachment 8975104 [details]
Bug 1460815 - Provide a chrome-only callback on CustomElementRegistry so script can define CE lazily

https://reviewboard.mozilla.org/r/243478/#review250490

::: dom/base/CustomElementRegistry.h:371
(Diff revisions 4 - 6)
>    static bool IsCustomElementEnabled(nsIDocument* aDoc);
>  
>    explicit CustomElementRegistry(nsPIDOMWindowInner* aWindow);
>  
> +private:
> +  class RunCustomElementCreationCallback : public mozilla::Runnable {

Nit, { goes to its own line

::: dom/base/CustomElementRegistry.h:379
(Diff revisions 4 - 6)
> +    explicit RunCustomElementCreationCallback(CustomElementRegistry* aRegistry,
> +                                              nsAtom* aAtom,
> +                                              CustomElementCreationCallback* aCallback)
> +      : mozilla::Runnable("CustomElementRegistry::RunCustomElementCreationCallback")
> +#ifdef DEBUG
> +//      , mRegistry(aRegistry)

What is this

::: dom/base/CustomElementRegistry.h:387
(Diff revisions 4 - 6)
> +      , mCallback(aCallback)
> +    {
> +    }
> +    private:
> +#ifdef DEBUG
> +      CustomElementRegistry* mRegistry;

and what is this. Remove these unused DEBUG only parts.

::: dom/base/CustomElementRegistry.cpp:312
(Diff revisions 4 - 6)
> +CustomElementRegistry::RunCustomElementCreationCallback::Run()
> +{
> +  ErrorResult er;
> +  nsDependentAtomString value(mAtom);
> +  mCallback->Call(value, er);
> +  MOZ_ASSERT(NS_SUCCEEDED(er.StealNSResult()));

Please add a comment why we do this. We could never enable this code to the web since we just fail on error. But asserting that chrome code is working the right way is probably ok

::: dom/base/CustomElementRegistry.cpp:315
(Diff revisions 4 - 6)
> +  nsDependentAtomString value(mAtom);
> +  mCallback->Call(value, er);
> +  MOZ_ASSERT(NS_SUCCEEDED(er.StealNSResult()));
> +
> +  MOZ_ASSERT(mRegistry->mCustomDefinitions.GetWeak(mAtom),
> +    "Callback should define the definition of type.");    

Ah, we add even this assertion. Perhaps this is ok, we'll see.

::: dom/base/CustomElementRegistry.cpp:315
(Diff revisions 4 - 6)
> +  nsDependentAtomString value(mAtom);
> +  mCallback->Call(value, er);
> +  MOZ_ASSERT(NS_SUCCEEDED(er.StealNSResult()));
> +
> +  MOZ_ASSERT(mRegistry->mCustomDefinitions.GetWeak(mAtom),
> +    "Callback should define the definition of type.");    

So we could just force upgrade here, no?
Perhaps a followup

::: dom/base/CustomElementRegistry.cpp:318
(Diff revisions 4 - 6)
> +
> +  MOZ_ASSERT(mRegistry->mCustomDefinitions.GetWeak(mAtom),
> +    "Callback should define the definition of type.");    
> +  MOZ_ASSERT(!mRegistry->mElementCreationCallbacks.GetWeak(mAtom), 
> +    "Callback should be removed.");
> +

You have some spaces in this method at the end of lines.
Attachment #8975104 - Flags: review?(bugs) → review+
Comment on attachment 8975104 [details]
Bug 1460815 - Provide a chrome-only callback on CustomElementRegistry so script can define CE lazily

https://reviewboard.mozilla.org/r/243478/#review250490

> What is this

Actually I need this for assertions (and I incorrectly commented out the line).
Review comment addressed. Thanks for the speedy review.

(In reply to Olli Pettay [:smaug] from comment #25)
> So we could just force upgrade here, no?
> Perhaps a followup

Let me see if this is easy to do (and don't require a followup); if so I will ask for an additional review here before landing.
ah, of course. But you can't have a raw pointer there. Use RefPtr
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #28)
> Let me see if this is easy to do (and don't require a followup); if so I
> will ask for an additional review here before landing.

OK. This is turned out to be pretty complicated.

I've try to call into CustomElementRegistry::UpgradeCandidates() but that just enqueue an upgrade action, so element returned from element.createElement() would still be an unupgraded one. Next thing I tried is to probe into the candidate map and try to CustomElementRegistry::Upgrade() them, but the elements created are not there; candidates are only added into the table from BindToTree(). So in order to do this I will need to keep track of elements created in another table, and have the runnable look into it after it gets the definition.

This needs to be worked on in another follow-up.
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #30)
> I've try to call into CustomElementRegistry::UpgradeCandidates() but that
> just enqueue an upgrade action, so element returned from
> element.createElement() would still be an unupgraded one. 

This statement is incorrect and the next sentence is the correct cause.
Pushed by timdream@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/410d3f74efb5
Provide a chrome-only callback on CustomElementRegistry so script can define CE lazily r=smaug
https://hg.mozilla.org/mozilla-central/rev/410d3f74efb5
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Type: enhancement → task
Regressions: 1760791
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: