Closed Bug 1404420 Opened 7 years ago Closed 7 years ago

Add Custom Element support for XUL Elements

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: bgrins, Assigned: mossop)

References

Details

Attachments

(3 files, 3 obsolete files)

xul-customelement-testcase.xul
635 bytes, application/vnd.mozilla.xul+xml
Details
Bug 1404420: Add custom element support to XUL.
59 bytes, text/x-review-board-request
edgar
: review+
smaug
: review+
Details
XUL custom element constructor stack
7.01 KB, text/plain
Details 
I'm not sure how much work this will be, but if we could at least prototype  support for Custom Elements via document.createElement in a XUL document for XUL elements that would help to compare performance with XBL in Bug 1387125 by letting me remove the CE polyfill from the benchmark.
Olli, would you be able to add some information about what we'd need to do to accomplish this? AIUI we'll at least need to add [CEReactions] on some XUL DOM APIs.
Flags: needinfo?(bugs)
That should be rather simple. Another one is to allow elements in xul namespace to be custom elements. 
And if we want also elements which don't have a dash in the name to be custom elements, some more tweaks are needed.

John, does anything else come to your mind.
Flags: needinfo?(bugs) → needinfo?(jdai)
(In reply to Olli Pettay [:smaug] from comment #2)
> And if we want also elements which don't have a dash in the name to be
> custom elements, some more tweaks are needed.

I would prefer that ultimately, but it's not needed here. I mainly want to get it working enough to gather some perf numbers in Bug 1387125, so even a patch that can be applied locally and does the bare minimum would be helpful.
At a quick look, you probably want to change the following:

1) Namespace check in nsContentUtils::SetupCustomElement.
2) AutoConstructionStackEntry assuming it maintains an array of HTML elements.

I thought there used to be bits that ensured that custom element constructors inherited from HTMLElement/SVGElement and returned sane things, but I can't quite find where that lives.
Just like Boris said, if we need to support Custom Elements via document.createElement in a XUL document for XUL elements. We need to support those two things. Also, we need to tweak custom element constructor to support XUL elements[1].

[1] http://searchfox.org/mozilla-central/rev/298033405057ca7aa5099153797467eceeaa08b5/dom/bindings/BindingUtils.cpp#3671-3678
Flags: needinfo?(jdai)
Priority: -- → P2
Ah, and http://searchfox.org/mozilla-central/rev/298033405057ca7aa5099153797467eceeaa08b5/dom/bindings/BindingUtils.cpp#3656-3660 for that last bit.  Basically, custom element registration currently uses local name only, because it presupposes HTML...
Simple testcase that can be saved and then run with:

./mach run --setpref dom.allow_XUL_XBL_for_file=true --setpref dom.webcomponents.customelements.enabled=true --setpref dom.webcomponents.enabled=true ~/Downloads/xul-customelement-testcase.xul
Assignee: nobody → dtownsend
Here's a patch that converts <tabbrowser> to a Custom Element. It seems to be having some issues with xpconnect for the following things:

* Can’t convert to an `nsIObserver` via `NS_ERROR_XPC_BAD_CONVERT_JS: Could not convert JavaScript argument arg 0 [nsIObserverService.addObserver]`: https://dxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#5888
* Can’t convert to an `nsIDOMEventListener` via `NS_ERROR_XPC_BAD_CONVERT_JS: Could not convert JavaScript argument arg 2 [nsIEventListenerService.addSystemEventListener]`: https://dxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#5895
* Can’t convert to an `nsIMessageListener` via `NS_ERROR_XPC_BAD_CONVERT_JS: Could not convert JavaScript argument arg 1 [nsIMessageListenerManager.addMessageListener]`: https://dxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#5938

With this line in the constructor:

this.QueryInterface = XPCOMUtils.generateQI([
  Ci.nsIObserver, Ci.nsISupportsWeakReference, Ci.nsIDOMEventListener, Ci.nsIMessageListener]);
Much simpler testcase for the problem in Comment 13 that doesn't require applying the <tabbrowser> patch:

`./mach run --setpref dom.webcomponents.customelements.enabled=true --setpref dom.webcomponents.enabled=true --jsconsole`

Paste this into the Browser Console:

  (function() {
    class Foo extends XULElement {
      constructor() {
        super()
        this.QueryInterface = XPCOMUtils.generateQI([Ci.nsIObserver,
                                           Ci.nsISupportsWeakReference]);
        Services.obs.addObserver(this, "contextual-identity-updated");
      }
    }
    customElements.define("firefox-foo", Foo);
    document.createElement("firefox-foo")
  })()

Results in: NS_ERROR_XPC_BAD_CONVERT_JS: Could not convert JavaScript argument arg 0 [nsIObserverService.addObserver]
You can't implement-in-JS extra interfaces on objects that represent a C++ object right now.  Specifically, if you have either a webidl or XPConnect-wrapped object, XPCConvert::JSObject2NativeInterface will just call QueryInterface on the underlying C++ object.  If your underlying C++ object is XPCWrappedJS to start with, then it will call your scripted QueryInterface, which is how that bit works.  But in this case the C++ object is a XULElement, so it just does XULElement::QueryInterface.

XBL has a hack into QueryInterface on elements; see the GetBindingImplementation() call in Element::QueryInterface.

For practical purposes, what you want is to set up a separate object that serves as your XPCOM interface implementation.  For simplicity, you can probably do:

  this.xpcomProxy = new Proxy(this, {});
  
and then pass this.xpcomProxy in all the cases where you need to implement XPCOM interfaces...
The latest patch mostly works for all the cases of instantiating a custom element in XUL. Adding support for parsed elements meant sending element creation through the NS_NewXULElement path. In some cases that I don't quite understand yet though this fails because the document isn't allowed to create XUL/XBL elements (http://searchfox.org/mozilla-central/source/dom/xul/nsXULElement.cpp#265). This is strange because the element being loaded at the time is an XBL document, it fails on this element: http://searchfox.org/mozilla-central/source/toolkit/themes/windows/global/globalBindings.xml#15. This causes us to crash later on when trying to add a null element to the document.

I'm not really sure of how to solve this. I could construct a path though NS_NewXULElement that ignores that safety check but it seems weird that we're hitting it. Any suggestions bz?
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #15)
> For practical purposes, what you want is to set up a separate object that
> serves as your XPCOM interface implementation.  For simplicity, you can
> probably do:
> 
>   this.xpcomProxy = new Proxy(this, {});
>   
> and then pass this.xpcomProxy in all the cases where you need to implement
> XPCOM interfaces...

Thanks - attaching a version patch that does so and indeed fixes these problems. Browser appears functional, will see what try has to say.
Attachment #8918436 - Attachment is obsolete: true
> it fails on this element: http://searchfox.org/mozilla-central/source/toolkit/themes/windows/global/globalBindings.xml#15.

<sigh>.  So chrome://*/skin/* stuff is not loaded with the system principal.  The "allowed to do XUL/XBL" check basically checks for system principal, if we ignore the file:// whitelist bits.

I think we basically have four options.  We could change nsContentUtils::AllowXULXBLForPrincipal to allow for chrome:// codebase principals.  We could change skin packages to use the system principal.  We could explicitly change nsDocument::InternalAllowXULXBL to check for an XBL document (via IsLoadedAsInteractiveData()).  Or we could have NS_NewXBLDocument call ForceEnableXULXBL() on the document it creates.  The last two options are basically equivalent in terms of security behavior: they enable XUL usage in all XBL documents. 

I think doing that last one (call ForceEnableXULXBL()) is probably fine.  Kinda wish we could run it by bholley, but we can ask him to take a look once he's back.
Flags: needinfo?(bzbarsky)
To use Custom Elements in the browser chrome we will need them always enabled in chrome code, regardless of the pref value of `dom.webcomponents.customelements.enabled`. This way if a user flips that pref off it won't break the browser.
(In reply to Boris Zbarsky [:bz] from comment #18)
>  We could change skin packages to use the system principal.

See also bug 1385444 to remove `skin` packages.
I did some talos pushes to get perf data about each part here:

+------------------+---------------------------------+------------------------+
| Base             | New                             | Results URL            |
+------------------+---------------------------------+------------------------+
| m-c              | CE Enabled                      | https://mzl.la/2gxqGKZ |
| m-c              | CE Enabled + XUL                | https://mzl.la/2zDZnDi |
| m-c              | CE Enabled + XUL + <tabbrowser> | https://mzl.la/2yvOmGD |
| CE Enabled       | CE Enabled + XUL                | https://mzl.la/2zovpCr |
| CE Enabled       | CE Enabled + XUL + <tabbrowser> | https://mzl.la/2ipuDlm |
| CE Enabled + XUL | CE Enabled + XUL + <tabbrowser> | https://mzl.la/2goA1B4 |
+------------------+---------------------------------+------------------------+

The regressions in the first row are due to just flipping the pref for Custom Elements - I filed Bug 1410536 for that. The few regressions in the fourth row *should* be capturing just the change for the patch here, but we'll probably have a more accurate picture using the new patch that enables CE only in chrome contexts.
Comment on attachment 8917525 [details]
Bug 1404420: Add custom element support to XUL.

https://reviewboard.mozilla.org/r/188476/#review197792

::: dom/base/CustomElementRegistry.h:191
(Diff revision 8)
>  
>    // The lifecycle callbacks to call for this custom element.
>    UniquePtr<mozilla::dom::LifecycleCallbacks> mCallbacks;
>  
>    // A construction stack. Use nullptr to represent an "already constructed marker".
> -  nsTArray<RefPtr<nsGenericHTMLElement>> mConstructionStack;
> +  nsTArray<RefPtr<mozilla::dom::Element>> mConstructionStack;

This is all inside the "mozilla::dom" namespace; you don't need that prefix.

::: dom/bindings/BindingUtils.h:3392
(Diff revision 8)
>  // be CheckUnwrapped, or if the global of the result has no docgroup
>  // (e.g. because it's not a Window global).
>  CustomElementReactionsStack*
>  GetCustomElementReactionsStack(JS::Handle<JSObject*> aObj);
>  // This function is expected to be called from the constructor function for an
>  // HTML element interface; the global/callargs need to be whatever was passed to

This comment probably needs adjusting.

::: dom/bindings/BindingUtils.h:3395
(Diff revision 8)
>  GetCustomElementReactionsStack(JS::Handle<JSObject*> aObj);
>  // This function is expected to be called from the constructor function for an
>  // HTML element interface; the global/callargs need to be whatever was passed to
>  // that constructor function.
> -already_AddRefed<nsGenericHTMLElement>
> +already_AddRefed<Element>
>  CreateHTMLElement(const GlobalObject& aGlobal, const JS::CallArgs& aCallArgs,

And the name of this function as well...

::: dom/bindings/BindingUtils.cpp:3607
(Diff revision 8)
>    int32_t tag = eHTMLTag_userdefined;
>    if (!definition->IsCustomBuiltIn()) {
>      // Step 4.
>      // If the definition is for an autonomous custom element, the active
> -    // function should be HTMLElement.
> -    JS::Rooted<JSObject*> constructor(cx, HTMLElementBinding::GetConstructorObject(cx));
> +    // function should be HTMLElement or XULElement
> +    JSObject* constructorObject;

Why do you need this non-rooted temporary?  Just use:

    JS::Rooted<JSObject*> constructor(cx);
    
and then assign to it.

::: dom/bindings/BindingUtils.cpp:3629
(Diff revision 8)
>    } else {
>      // Step 5.
>      // If the definition is for a customized built-in element, the localName
>      // should be defined in the specification.
> +
> +    // Customized build-in elements are not supported for XUL yet.

"built-in"

::: dom/bindings/BindingUtils.cpp:3666
(Diff revision 8)
>    }
>  
>    RefPtr<mozilla::dom::NodeInfo> nodeInfo =
>      doc->NodeInfoManager()->GetNodeInfo(definition->mLocalName,
>                                          nullptr,
> -                                        kNameSpaceID_XHTML,
> +                                        ns,

This doesn't look right.  We want to use the HTML namespace here if we're not using XUL, but there's no guarantee that "ns" won't be something else (most particularly kNamSpaceID_None).

::: dom/xbl/nsXBLService.cpp:1032
(Diff revision 8)
>  
>    // Create document and contentsink and set them up.
>    nsCOMPtr<nsIDocument> doc;
>    rv = NS_NewXMLDocument(getter_AddRefs(doc));
>    NS_ENSURE_SUCCESS(rv, rv);
> +  doc->ForceEnableXULXBL();

This needs a comment explaining why it's there.

::: dom/xml/XMLDocument.cpp:239
(Diff revision 8)
>                                    nullptr, DocumentFlavorLegacyGuess,
>                                    aStyleBackend);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
>    nsCOMPtr<nsIDocument> idoc = do_QueryInterface(*aInstancePtrResult);
> +  idoc->ForceEnableXULXBL();

This needs a comment as well.

::: dom/xul/nsXULElement.cpp:264
(Diff revision 8)
>  
>      return NS_OK;
>  }
>  
> +static void
> +DoCustomElementCreate(Element** aElement, nsIDocument* aDoc,

There seems to be a lot of copy/paste here from the HTML case.  Is the code actually different?  Can we share it?
Comment on attachment 8917525 [details]
Bug 1404420: Add custom element support to XUL.

https://reviewboard.mozilla.org/r/188476/#review197792

> There seems to be a lot of copy/paste here from the HTML case.  Is the code actually different?  Can we share it?

There is just one difference, the check for IsHTMLElement in the middle of the function. I could break that outside the function and then move the rest somewhere shared but I'm not sure where would be appropriate.
Flags: needinfo?(bzbarsky)
Blocks: 1411707
A few issues I noticed when using these patches in Bug 1411707, where I convert <stringbundle> from XBL to a Custom Element:

1) This doesn't turn Custom Elements on by default in in-content chrome pages. STR: open `about:config`, open web console, enter `customElements`, get `ReferenceError: customElements is not defined`
2) There is a leak whenever a Custom Element class defines a `disconnectedCallback`. For instance, if I add this to browser.js and then run any mochitest, i.e. `./mach mochitest browser/base/content/test/sidebar/browser_sidebar_switcher.js` I get "WARNING: YOU ARE LEAKING THE WORLD (at least one JSRuntime and everything alive inside it, that is) AT JS_ShutDown TIME.  FIX THIS!":

```
  class FirefoxStringbundle extends XULElement {
    disconnectedCallback() { }
  }
  customElements.define("firefox-stringbundle", FirefoxStringbundle);
  document.addEventListener("DOMContentLoaded", () => {
    document.documentElement.appendChild(document.createElement("firefox-stringbundle"));
  });
```

But if I simply remove `disconnectedCallback() {}` from the class the leak goes away.  I'm not sure if this is something browser-specific or if it's a potential leak for content as well.

3) In some tests I see a crash with "GECKO(1529) | Assertion failure: !aReaction->IsUpgradeReaction() (Upgrade reaction should not be scheduled to backup queue), at /builds/worker/workspace/build/src/dom/base/CustomElementRegistry.cpp:1084": https://treeherder.mozilla.org/#/jobs?repo=try&revision=e775f002f69599316e6bfc4bb35c5d3f64fdea74&selectedJob=139641294
(In reply to Brian Grinstead [:bgrins] from comment #30)
> 1) This doesn't turn Custom Elements on by default in in-content chrome
> pages. STR: open `about:config`, open web console, enter `customElements`,
> get `ReferenceError: customElements is not defined`

So I guess there is an open question around where we want this enabled. Is it only for privileged documents (top-level windows, about:addons etc), or is it also for unprivileged content that is loaded from the app (about:home, about:newtab, etc.).

Either way I guess I'm not using the right test here.
> but I'm not sure where would be appropriate

nsContentUtils seems reasonable.

> doesn't turn Custom Elements on by default in in-content chrome pages.

That's because the "enable in chrome" patch is testing for a chrome window, not a system-principal caller.  Seems to me like it should test for a system-principal caller.

> There is a leak whenever a Custom Element class defines a `disconnectedCallback`

Hmm.  Does this apply only to `disconnectedCallback` or the other callbacks too?  They are all handled pretty similarly....
Flags: needinfo?(bzbarsky)
> or is it also for unprivileged content that is loaded from the app (about:home, about:newtab, etc.)

That is a good question, yes...  We should sort that out.
(In reply to Dave Townsend [:mossop] from comment #31)
> So I guess there is an open question around where we want this enabled. Is
> it only for privileged documents (top-level windows, about:addons etc), or
> is it also for unprivileged content that is loaded from the app (about:home,
> about:newtab, etc.).

Do about:home / about:newtab use XBL?  I'm assuming no if they are unprivileged - and if they don't then I don't think we need Custom Element support
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #32)
> > There is a leak whenever a Custom Element class defines a `disconnectedCallback`
> 
> Hmm.  Does this apply only to `disconnectedCallback` or the other callbacks
> too?  They are all handled pretty similarly....

It doesn't apply to connectedCallback or attributeChangedCallback - I only see it when adding disconnectedCallback.

By the way, I wanted to confirm that the behavior I've noticed is expected:

- With XBL, the <destructor> does fire when a document is unloaded with the element still in the DOM
- With Custom Elements, the disconnectedCallback doesn't fire when the document is unloaded with the element still in the DOM

If that is correct, would there be a good way to simulate a disconnectedCallback or introduce a separate callback so that we can handle cases like <tabbrowser> where we need to remove observers when the window is closed?  Or do you think this is something where we'll need to keep track of connected components in the frontend and manually call disconnectedCallback when the window unload event fires?
Flags: needinfo?(bzbarsky)
> I only see it when adding disconnectedCallback.

That's ... really weird.  It's worth checking whether we can reproduce that with a pretty minimal document...

> By the way, I wanted to confirm that the behavior I've noticed is expected:

That's probably a question for Edgar.  I haven't checked what the disconnectedCallback semantics are recently.  That said, since navigating away from the document technically doesn't actually disconnect its DOM...

We could probably add some sort of chromeonly callback bit, if we can figure out the right semantics for it.
Flags: needinfo?(bzbarsky) → needinfo?(echen)
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #32)
> > but I'm not sure where would be appropriate
> 
> nsContentUtils seems reasonable.

I just realised I'd have to pull out half the function leaving only a small bit to unduplicate. I'm not sure it is worth it.
I wasn't just talking about DoCustomElementCreate, but also the extensive duplication between NS_NewXULElement and NS_NewHTMLElement of some pretty finicky code...
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #36)
> That's probably a question for Edgar.  I haven't checked what the
> disconnectedCallback semantics are recently.  That said, since navigating
> away from the document technically doesn't actually disconnect its DOM...

Per spec, disconnectedCallback is fired when removing a node from a parent, see https://dom.spec.whatwg.org/#concept-node-remove.
It seems navigating away from the document doesn't have disconnectedCallback fired is expected.
Flags: needinfo?(echen)
(In reply to Brian Grinstead [:bgrins] from comment #30)
> 3) In some tests I see a crash with "GECKO(1529) | Assertion failure:
> !aReaction->IsUpgradeReaction() (Upgrade reaction should not be scheduled to
> backup queue), at
> /builds/worker/workspace/build/src/dom/base/CustomElementRegistry.cpp:1084":
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=e775f002f69599316e6bfc4bb35c5d3f64fdea74&selectedJob=1
> 39641294

Upgrade reactions should not be scheduled to backup queue per spec, so we will hit this assertion if we miss AutoCEReaction on the codepath which will trigger enqueueing an upgrade reaction.

In the crash case, the upgrade reaction is from http://searchfox.org/mozilla-central/rev/40e8eb46609dcb8780764774ec550afff1eed3a5/dom/xbl/nsXBLService.cpp#506. We probably could add AutoCEReaction (Before nsAutoScriptBlocker) if the enqueueing upgrade reaction is expected.
(In reply to Brian Grinstead [:bgrins] from comment #35)
> It doesn't apply to connectedCallback or attributeChangedCallback - I only
> see it when adding disconnectedCallback.

I can reproduce the leak with the same rev of try push of bug 1411707 comment #4. I will take a look. ni? to me for tracking.
Flags: needinfo?(echen)
> if the enqueueing upgrade reaction is expected

Anything that adds elements to the DOM can enqueue and upgrade reaction, right?  If so, then it's expected in this case.
Attachment #8920691 - Flags: review?(echen)
I've updated the main patch with bz's review comments and that is now ready for review. The patch to always enable custom elements for privileged windows still isn't working and I haven't been able to figure out why. bz can you suggest what I should be checking instead? With the current patch window.customElements doesn't exist for any windows unless the prefs are set. What we're looking for is to have it work for privileged windows.
Flags: needinfo?(bzbarsky)
What is our definition of "privileged windows"?
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #46)
> What is our definition of "privileged windows"?

We care about any window that can load XUL. I would do it just for XUL documents but it looks like we call the function before the document is available to check that on.
> We care about any window that can load XUL.

nsContentUtils::AllowXULXBLForPrincipal on the window object's principal, then.
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #48)
> > We care about any window that can load XUL.
> 
> nsContentUtils::AllowXULXBLForPrincipal on the window object's principal,
> then.

Am I getting the window principal correctly? It is sometimes nullptr and other than that nsContentUtils::AllowXULXBLForPrincipal doesn't seem to pass for the main browser XUL window.
> Am I getting the window principal correctly?

No, because GetPrincipal() delegates to the document, but this is very early on in the setup process, before we've told the window what its document is.

What you probably want is nsContentUtils::ObjectPrincipal(aObject).
Attachment #8920691 - Flags: review?(echen)
(In reply to Edgar Chen [:edgar] from comment #41)
> (In reply to Brian Grinstead [:bgrins] from comment #35)
> > It doesn't apply to connectedCallback or attributeChangedCallback - I only
> > see it when adding disconnectedCallback.
> 
> I can reproduce the leak with the same rev of try push of bug 1411707
> comment #4. I will take a look. ni? to me for tracking.

The leak is from dispatching a runnable to micro task queue (for processing back queue) during shutdown, file bug 1413418.
Flags: needinfo?(echen)
Comment on attachment 8917525 [details]
Bug 1404420: Add custom element support to XUL.

https://reviewboard.mozilla.org/r/188476/#review198506

CustomElement changes look good to me, but I would like to take a look again on an rebased version for bug 1406325 and invite smaug to review.
smaug, would you mind to review this?

::: dom/base/CustomElementRegistry.cpp:194
(Diff revision 9)
>  {
>  public:
> -  AutoConstructionStackEntry(nsTArray<RefPtr<nsGenericHTMLElement>>& aStack,
> -                             nsGenericHTMLElement* aElement)
> +  AutoConstructionStackEntry(nsTArray<RefPtr<Element>>& aStack,
> +                             Element* aElement)
>      : mStack(aStack)
>    {

Could you add assertion to ensure the aElement should be a HTMLElement or XULElement? Thank you.

```
MOZ_ASSERT(aElement->IsHTMLElement() || aElement->IsXULElement());
```

::: dom/base/nsContentUtils.cpp:10260
(Diff revision 9)
> +    (*aResult)->SetCustomElementData(new CustomElementData(definition->mType));
> +    nsContentUtils::EnqueueUpgradeReaction(*aResult, definition);
> +    return NS_OK;
> +  }
> +
> +  bool isCustomElementName;

Nit: Assign an initial value

::: dom/html/nsHTMLContentSink.cpp
(Diff revision 9)
> -    return NS_ERROR_OUT_OF_MEMORY;
> -  }
> -
> -  if (CustomElementRegistry::IsCustomElementEnabled() &&
> -      (isCustomElementName || aIs)) {
> -    nsContentUtils::SetupCustomElement(*aResult, aIs);

SetupCustomElement are going to be removed in bug 1406325.

::: dom/tests/mochitest/webcomponents/test_xul_custom_element.xul:34
(Diff revision 9)
> +    function runTest() {
> +      const XUL_NS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
> +
> +      let element = document.createElementNS(XUL_NS, "test-custom-element");
> +      document.querySelector("#content").appendChild(element);
> +      is(element.textContent, "foo", "Should have set the textContent");

It will be good if we could also add checks for element's prototype.

::: dom/webidl/XULElement.webidl:11
(Diff revision 9)
>  
>  interface XULControllers;
>  interface MozRDFCompositeDataSource;
>  interface MozRDFResource;
>  
> -[Func="IsChromeOrXBL"]
> +[HTMLConstructor, Func="IsChromeOrXBL"]

WEBIDL changes need DOM peer's review.

::: dom/xbl/nsXBLService.cpp:1036
(Diff revision 9)
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  // XBL documents must allow XUL and XBL elements in them but the usual check
> +  // only checks if the document is loaded in the system principal which is
> +  // sometimes not the case.
> +  doc->ForceEnableXULXBL();

I'm not competent to review this bit.
Attachment #8917525 - Flags: review?(echen)
Attachment #8917525 - Flags: review?(bugs)
Comment on attachment 8917525 [details]
Bug 1404420: Add custom element support to XUL.

https://reviewboard.mozilla.org/r/188476/#review198506

> It will be good if we could also add checks for element's prototype.

I added an instanceof TestCustomElement check which seems to work. I couldn't see how the element's prototype relates to the class itself, maybe I'm misunderstanding something about how classes and prototypes mesh in JS.
Attachment #8917525 - Flags: review?(bugs)
So at which point do the Custom Element constructors run when xul document is loaded from fastloadcache/xul-proto?
(In reply to Olli Pettay [:smaug] from comment #57)
> So at which point do the Custom Element constructors run when xul document
> is loaded from fastloadcache/xul-proto?

That's a good question. Brian has been testing with this patch and loading new browser windows with parsed custom elements in them fine. Does that mean it's working? I'm not sure.

Is there anyway we can test the behaviour when loaded from the cache?
Brian, have you been running debug builds? Any new warnings or assertions firing?

Also, some stack traces about when callbacks get run wouldn't hurt, need to review that those places are ok for running random scripts.
Comment on attachment 8917525 [details]
Bug 1404420: Add custom element support to XUL.

https://reviewboard.mozilla.org/r/188476/#review201678

Looks reasonable, but could take another look, especially if one could explain the stack traces we'll get when loading XUL (to which CE is attached) from fastload.
Also, does the patch somewhere guarantee that we don't end up trying to use XBL and CE on same XUL elements?

::: dom/base/nsContentUtils.h:3028
(Diff revision 10)
>     * https://html.spec.whatwg.org/#concept-document-https-state
>     * https://fetch.spec.whatwg.org/#concept-response-https-state
>     */
>    static bool HttpsStateIsModern(nsIDocument* aDocument);
>  
> +  static nsresult NewElement(Element** aResult, mozilla::dom::NodeInfo* aNodeInfo,

So this would have a different name and then add a comment here what it actually does.

::: dom/base/nsContentUtils.cpp:10147
(Diff revision 10)
> +
> +  element.forget(aElement);
> +}
> +
> +/* static */ nsresult
> +nsContentUtils::NewElement(Element** aResult, mozilla::dom::NodeInfo* aNodeInfo,

This method needs to be called something else hinting that this is all about custom elements, or at least it is all about HTML and XUL elements, but not about any other elements.

Perhaps just NewXULOrHTMLElement, and then assert hard that passed nodeinfo has the right namespace

::: dom/bindings/BindingUtils.cpp:3554
(Diff revision 10)
>    return docGroup->CustomElementReactionsStack();
>  }
>  
>  // https://html.spec.whatwg.org/multipage/dom.html#htmlconstructor
> -already_AddRefed<nsGenericHTMLElement>
> -CreateHTMLElement(const GlobalObject& aGlobal, const JS::CallArgs& aCallArgs,
> +already_AddRefed<Element>
> +CreateElement(const GlobalObject& aGlobal, const JS::CallArgs& aCallArgs,

This needs to have some different name hinting that HTML or XUL element is created

::: dom/xul/nsXULElement.cpp:193
(Diff revision 10)
>      RefPtr<mozilla::dom::NodeInfo> ni = aNodeInfo;
> -    RefPtr<nsXULElement> element = new nsXULElement(ni.forget());
> -    if (element) {
> +    nsCOMPtr<Element> baseElement;
> +    NS_NewXULElement(getter_AddRefs(baseElement), ni.forget(), dom::FROM_PARSER_NETWORK, nullptr);
> +
> +    if (baseElement) {
> +        RefPtr<nsXULElement> element = FromContent(baseElement);

Why RefPtr? It ends up doing extra addref/release

::: dom/xul/nsXULElement.cpp:224
(Diff revision 10)
>                  }
>              }
>          }
> -    }
>  
> -    return element.forget();
> +        return element.forget();

And then you can do something like
return baseElement.forget().downcast<nsXULElement>()

::: dom/xul/nsXULElement.cpp
(Diff revision 10)
> -    RefPtr<mozilla::dom::NodeInfo> ni = aNodeInfo;
> +    RefPtr<mozilla::dom::NodeInfo> nodeInfo = aNodeInfo;
> -
> -    NS_PRECONDITION(ni, "need nodeinfo for non-proto Create");
>  
> -    nsIDocument* doc = ni->GetDocument();
> +    NS_PRECONDITION(nodeInfo, "need nodeinfo for non-proto Create");
> -    if (doc && !doc->AllowXULXBL()) {

We can't drop this.
Attachment #8917525 - Flags: review?(bugs) → review-
Attachment #8918474 - Attachment is obsolete: true
(In reply to Olli Pettay [:smaug] from comment #59)
> Brian, have you been running debug builds? Any new warnings or assertions
> firing?

I had been running opt builds.  Used a debug build today and I don't see anything out of the ordinary either at startup or when opening a new window. Not really sure what to look for though.

> Also, some stack traces about when callbacks get run wouldn't hurt, need to
> review that those places are ok for running random scripts.

How can I get the stack traces you care about? I'm assuming you mean from the CE code itself, because from the frontend if I do console trace() in connectedCallback I don't see anything too interesting, just pointing back to the CE registration with an async frame in between:

  connectedCallback chrome://global/content/customelements.js:10:5
  (Async: LifecycleConnectedCallback) <-- Not sure about this, there's no location associated with it
  <anonymous> chrome://global/content/customelements.js:89:1 <-- The call to customElements.define()
  BG_observe browser/components/nsBrowserGlue.js:341:9 <-- The thing that triggers subscript loader
Flags: needinfo?(bugs)
Could you add breakpoints to the code which executes CE constructor and capture the stacks?
And I'm talking about native stack traces.
Flags: needinfo?(bugs)
I added a break to the constructor for the stringbundle element that Brian is working on and this is the stack we get. Do you want stacks from any other places smaug?
Flags: needinfo?(bugs)
Thanks. That doesn't look too scary stack, given that also scripts may run around XULDocument.cpp:2783
Flags: needinfo?(bugs)
I've rebased this on top of bug 1406325 and addressed the comments so far. This is ready for review now.
(mozreview makes reviewing this kinds of patches so much more annoying.)
Comment on attachment 8917525 [details]
Bug 1404420: Add custom element support to XUL.

https://reviewboard.mozilla.org/r/188476/#review206506

Could you test something like https://bug934607.bmoattachments.org/attachment.cgi?id=8872948 to see whether this affects to the performance
(when custom elements are enabled).

::: dom/base/nsContentUtils.cpp:10198
(Diff revision 11)
> +  int32_t tag = eHTMLTag_unknown;
> +  if (nodeInfo->NamespaceEquals(kNameSpaceID_XHTML)) {
> +    tag = nsHTMLTags::CaseSensitiveAtomTagToId(name);
> +  }
> +
> +  nsIDocument* doc = nodeInfo->GetDocument();

I don't know why you added this local variable in middle of a method which was otherwise mostly just moved + xul namespace checks added.
Yet you still use nodeInfo->GetDocument() too.

With this local variable one now needs to prove that it can't ever point to a deleted object.
So, if there wasn't any good reason, just keep using nodeInfo->GetDocument()

::: dom/bindings/BindingUtils.cpp:3631
(Diff revision 11)
>      // If the definition is for a customized built-in element, the localName
>      // should be defined in the specification.
> +
> +    // Customized built-in elements are not supported for XUL yet.
> +    if (ns == kNameSpaceID_XUL) {
> +      aRv.Throw(NS_ERROR_UNEXPECTED);

Perhaps throw NOT_SUPPORTED error
Attachment #8917525 - Flags: review?(bugs) → review+
Comment on attachment 8917525 [details]
Bug 1404420: Add custom element support to XUL.

https://reviewboard.mozilla.org/r/188476/#review207172

::: dom/base/nsContentCreatorFunctions.h:64
(Diff revision 11)
>  #ifdef MOZ_XUL
>  nsresult
>  NS_NewXULElement(mozilla::dom::Element** aResult,
> -                 already_AddRefed<mozilla::dom::NodeInfo>&& aNodeInfo);
> +                 already_AddRefed<mozilla::dom::NodeInfo>&& aNodeInfo,
> +                 mozilla::dom::FromParser aFromParser,
> +                 const nsAString* aIs);

We probably don't need `aIs` since we don't support customized built-in element for XULElement for now.

::: dom/base/nsContentUtils.cpp:10185
(Diff revision 11)
> +nsContentUtils::NewXULOrHTMLElement(Element** aResult, mozilla::dom::NodeInfo* aNodeInfo,
> +                                    FromParser aFromParser, const nsAString* aIs,
> +                                    mozilla::dom::CustomElementDefinition* aDefinition)
> +{
> +  RefPtr<mozilla::dom::NodeInfo> nodeInfo = aNodeInfo;
> +  NS_ASSERTION(nodeInfo->NamespaceEquals(kNameSpaceID_XHTML) ||

s/NS_ASSERTION/MOZ_ASSERT/

::: dom/base/nsContentUtils.cpp:10257
(Diff revision 11)
> +      // SetCustomElementData().
> +      // Built-in element
> +      if (nodeInfo->NamespaceEquals(kNameSpaceID_XHTML)) {
> +        *aResult = CreateHTMLElement(tag, nodeInfo.forget(), aFromParser).take();
> +      } else {
> +        *aResult = new nsXULElement(nodeInfo.forget());

We don't support customized built-in element for XULElement yet, so it seems that it isn't possible to be here. Maybe we should return an error or assert it.

::: dom/base/nsNodeUtils.cpp:467
(Diff revision 11)
>        aError.Throw(rv);
>        return nullptr;
>      }
>  
>      if (CustomElementRegistry::IsCustomElementEnabled() &&
> -        clone->IsHTMLElement()) {
> +        clone->IsElement()) {

`clone->IsHTMLElement || clone->IsXULElement` then we don't try to get 'is' attribute on other elements, e.g. SVGElement.
Attachment #8917525 - Flags: review?(echen) → review+
Comment on attachment 8920691 [details]
Bug 1404420: Always enable custom elements in chrome.

https://reviewboard.mozilla.org/r/191686/#review207188

The changes look good to me, but there are still some perfmance work (bug 1396567), we should at least ensure enabling custom elements (event if in chrome only) won't cause prefmance regression on _normal_ case (i.e. not using custom elements feature). I am wondering if we could separate this part to a different bug?
Attachment #8920691 - Flags: review?(echen)
(In reply to Edgar Chen [:edgar] from comment #71)
> Comment on attachment 8920691 [details]
> Bug 1404420: Always enable custom elements in chrome.
> 
> https://reviewboard.mozilla.org/r/191686/#review207188
> 
> The changes look good to me, but there are still some perfmance work (bug
> 1396567), we should at least ensure enabling custom elements (event if in
> chrome only) won't cause prefmance regression on _normal_ case (i.e. not
> using custom elements feature). I am wondering if we could separate this
> part to a different bug?

I've filed bug 1421070 for enabling this in chrome code.
Pushed by dtownsend@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/97e76d278a23
Add custom element support to XUL. r=edgar, r=smaug
Attachment #8920691 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/97e76d278a23
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Blocks: 1421070
Blocks: 1444285
Component: DOM → DOM: Core & HTML
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: