981418 - CustomizableUI type:view and type:button widgets need an onBeforeCreated handler

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Mindaugas <LA-MJ>]

This bug affects type:view widgets.

There appears to be an inherent race condition with view panel creation in the window listener of bootstrapped add-ons.

When new windows are opened add-on code in the window listener(s) and CustomizableUI code run independently. 

However, CustomizableUI needs a given view to exist before buildWidget is executed: http://lxr.mozilla.org/mozilla-aurora/source/browser/components/customizableui/src/CustomizableUI.jsm#1125

This means that if add-on code that creates the relevant view is sufficiently delayed for any reason, CustomizableUI fails to create the widget with the following error:
console.error:
[CustomizableUI]
Could not find the view node with id: myapp-view, for widget: myapp-toolbar-button.

There is an onBuild handler intended for custom widgets only.
Specifying type:custom and rewriting type:view code in onBuild() is not an ideal solution for what I hope are obvious reasons.

It seems to me that this should be fixed at the platform level. Namely, there should be an onBeforeBuild handler (at http://lxr.mozilla.org/mozilla-aurora/source/browser/components/customizableui/src/CustomizableUI.jsm#1075) to allow creating panelviews in time regardless of any circumstances.

Comment 1

[:Gijs (out for now; he/him)]

I considered adding this for bug 978249, but Jorge reported that using DOMContentLoaded works. Why doesn't it work for you?

Comment 2

[Mindaugas <LA-MJ>]

Right, I failed to find that bug, sorry. Changing the window listener indeed solves it, thank you.

This should be documented somewhere I think.

Comment 3

[Mindaugas <LA-MJ>]

Actually, this does not solve the problem. It works for opening new windows, for sure. But when disabling/enabling add-on with already loaded windows, CustomizableUI still races ahead of panelview creation.

Comment 4

[Mindaugas <LA-MJ>]

juggling this singleton and per-window code is a pain

Comment 5

[:Gijs (out for now; he/him)]

(In reply to 4mr.minj from comment #3)
> Actually, this does not solve the problem. It works for opening new windows,
> for sure. But when disabling/enabling add-on with already loaded windows,
> CustomizableUI still races ahead of panelview creation.

Presumably you just shouldn't be calling createWidget before looping through the windows and having ensured that your panelview is in there... that doesn't seem terrible (and has little to do with what handlers you use for windows that haven't loaded yet).

In any case, maybe we should consider adding this. Blair, thoughts?

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Yea, we should definitely support this. Add-ons can do it via load events and when adding the widget - but they shouldn't have to if we can make it easier. And the view nodes may never need to exist - if we can easily support lazily creating the view, we should (anything to help reduce what we do on new window startup).

Comment 7

[:Gijs (out for now; he/him)]

Attachment: add onBeforeBuild handler for Australis widget,

onViewShowing can be used to do more lazy building. I think this is the simplest way to fix the problem. Adding more laziness would be possible, but comes with tracking in which windows we've ensured that the widget's view has the appropriate classes and handles. Additionally, not setting these immediately when the view is created might cause display issues in the multiview (which is obviously theoretical right now, but after the issues with the SDK widgets I'd rather not risk it). Jared, does this look right to you?

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8388901 [details] [diff] [review]
add onBeforeBuild handler for Australis widget,

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

r=me with the issues below fixed.

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ -1082,5 @@
>      let node;
>      if (aWidget.type == "custom") {
>        if (aWidget.onBuild) {
> -        try {
> -          node = aWidget.onBuild(aDocument);

i checked and the removal of this is OK since these functions are wrapped by wrapWidgetEventHandler which puts a try/catch around the add-on supplied code.

@@ +1088,5 @@
>          ERROR("Custom widget with id " + aWidget.id + " does not return a valid node");
>      }
>      else {
> +      if (aWidget.onBeforeBuild) {
> +        aWidget.onBeforeBuild(aDocument);

This function should be renamed to onBeforeCreated since it never precedes onBuild, but does precede onCreated.

@@ +1971,5 @@
>      widget.disabled = aData.disabled === true;
>  
>      this.wrapWidgetEventHandler("onClick", widget);
>      this.wrapWidgetEventHandler("onCreated", widget);
> +    this.wrapWidgetEventHandler("onBeforeBuild", widget);

Alphabetical ordering here plz?

::: browser/components/customizableui/test/browser_981418-widget-onbeforebuild-handler.js
@@ +7,5 @@
> +
> +// Should be able to add broken view widget
> +add_task(function testAddOnBeforeBuildWidget() {
> +  let viewShownDeferred = Promise.defer();
> +  let onBeforeBuildCalled = false;

*don't forget to update variable names like this*

@@ +55,5 @@
> +      tempPanel.hidePopup();
> +      yield panelHiddenPromise;
> +
> +
> +      CustomizableUI.addWidgetToArea(kWidgetId, CustomizableUI.AREA_PANEL);

nit, can remove one of these blank lines.

Comment 9

[:Gijs (out for now; he/him)]

Landed with nits, updated docs in CustomizableUI.jsm as well as https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/CustomizableUI.jsm/API-provided_widgets .

remote:   https://hg.mozilla.org/integration/fx-team/rev/fdebf60103c3

Comment 10

[:Gijs (out for now; he/him)]

Followup for test failures: https://hg.mozilla.org/integration/fx-team/rev/f66e3ccb0c41

Comment 11

[:Gijs (out for now; he/him)]

Backed out because of persistent orange:

remote:   https://hg.mozilla.org/integration/fx-team/rev/ac800841c853

Comment 12

[Mindaugas <LA-MJ>]

Thank you for doing this. After fixing the previous issues I encountered 
> viewNode is null panelUI.xml:186
when the button is the AREA_PANEL. 

It works fine initially after I move it there but not if I restart Firefox after that.

Comment 13

[:Gijs (out for now; he/him)]

Attachment: add onBeforeCreated handler for Australis widgets so add-ons have warning when their widget is created, and fix a race condition in the panelmultiview code that showed up in the automated test,

This wfm on my windows machine where I could reproduce the original error, but I'll push to try in a bit.

Comment 14

[:Gijs (out for now; he/him)]

(In reply to 4mr.minj from comment #12)
> Thank you for doing this. After fixing the previous issues I encountered 
> > viewNode is null panelUI.xml:186
> when the button is the AREA_PANEL. 
> 
> It works fine initially after I move it there but not if I restart Firefox
> after that.

I'm not sure what you mean. Can you elaborate in a separate bug (ideally with code that triggers this issue, and a stack trace / STR)? It seems orthogonal to this issue.

Comment 15

[:Gijs (out for now; he/him)]

 https://tbpl.mozilla.org/?tree=Try&rev=cf23978927fb

Comment 16

[Mindaugas <LA-MJ>]

(In reply to :Gijs Kruitbosch from comment #14)
> (In reply to 4mr.minj from comment #12)
> > Thank you for doing this. After fixing the previous issues I encountered 
> > > viewNode is null panelUI.xml:186
> > when the button is the AREA_PANEL. 
> > 
> > It works fine initially after I move it there but not if I restart Firefox
> > after that.
> 
> I'm not sure what you mean. Can you elaborate in a separate bug (ideally
> with code that triggers this issue, and a stack trace / STR)? It seems
> orthogonal to this issue.

Disregard my post. I appended my view not to #PanelUI-multiView but elsewhere. Hence why got 'view not found'.
I was mislead by an erroneous example on FF add-ons blog.

Comment 17

[:Gijs (out for now; he/him)]

Upping per discussion on IRC.

Comment 18

[:Gijs (out for now; he/him)]

(In reply to :Gijs Kruitbosch from comment #15)
>  https://tbpl.mozilla.org/?tree=Try&rev=cf23978927fb

Try looks great. :-)

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8389508 [details] [diff] [review]
add onBeforeCreated handler for Australis widgets so add-ons have warning when their widget is created, and fix a race condition in the panelmultiview code that showed up in the automated test,

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

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +2727,5 @@
>     * - onBuild(aDoc): Only useful for custom widgets (and required there); a
>     *                  function that will be invoked with the document in which
>     *                  to build a widget. Should return the DOM node that has
>     *                  been constructed.
> +   * - onBeforeCreated(aDoc): Attached to all non-custom widgets; a function

Should we say which functions are optional?

Comment 20

[:Gijs (out for now; he/him)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #19)
> Comment on attachment 8389508 [details] [diff] [review]
> add onBeforeCreated handler for Australis widgets so add-ons have warning
> when their widget is created, and fix a race condition in the panelmultiview
> code that showed up in the automated test,
> 
> Review of attachment 8389508 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/customizableui/src/CustomizableUI.jsm
> @@ +2727,5 @@
> >     * - onBuild(aDoc): Only useful for custom widgets (and required there); a
> >     *                  function that will be invoked with the document in which
> >     *                  to build a widget. Should return the DOM node that has
> >     *                  been constructed.
> > +   * - onBeforeCreated(aDoc): Attached to all non-custom widgets; a function
> 
> Should we say which functions are optional?

All of them except onBuild are optional, which is only required/useful for custom widgets. So I didn't think it was useful to state it every time. Can you think of some other way to clarify this?

Comment 21

[:Gijs (out for now; he/him)]

And into the fray once more:

remote:   https://hg.mozilla.org/integration/fx-team/rev/c8a1458bfe7d

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c8a1458bfe7d

Comment 23

[:Gijs (out for now; he/him)]

Comment on attachment 8389508 [details] [diff] [review]
add onBeforeCreated handler for Australis widgets so add-ons have warning when their widget is created, and fix a race condition in the panelmultiview code that showed up in the automated test,

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis
User impact if declined: add-ons will struggle to dynamically create views for their widgets; small correctness fix to panels when closing them quickly
Testing completed (on m-c, etc.): on m-c, locally, has automated test
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

Comment 24

[:Gijs (out for now; he/him)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/aa6888a6a048