Mindaugas <LA-MJ> Reporter
	Description • 11 years ago

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.

	:Gijs (he/him) Assignee
	Comment 1 • 11 years ago

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

	Mindaugas <LA-MJ> Reporter
	Comment 2 • 11 years ago

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

This should be documented somewhere I think.

	Mindaugas <LA-MJ> Reporter
	Comment 3 • 11 years ago

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.

	Mindaugas <LA-MJ> Reporter
	Comment 4 • 11 years ago

juggling this singleton and per-window code is a pain

	:Gijs (he/him) Assignee
	Comment 5 • 11 years ago

(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?

	Blair McBride [:Unfocused] (UNAVAILABLE)
	Comment 6 • 11 years ago

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).

	:Gijs (he/him) Assignee
	Comment 7 • 11 years ago

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?

	Jared Wein [:jaws] (please needinfo? me)
	Comment 8 • 11 years ago

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.

	:Gijs (he/him) Assignee
	Comment 9 • 11 years ago

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

	:Gijs (he/him) Assignee
	Comment 10 • 11 years ago

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

	:Gijs (he/him) Assignee
	Comment 11 • 11 years ago

Backed out because of persistent orange:

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

	Mindaugas <LA-MJ> Reporter
	Comment 12 • 11 years ago

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.

	:Gijs (he/him) Assignee
	Comment 13 • 11 years ago

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

	:Gijs (he/him) Assignee
	Comment 14 • 11 years ago

(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.

	:Gijs (he/him) Assignee
	Comment 15 • 11 years ago

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

	Mindaugas <LA-MJ> Reporter
	Comment 16 • 11 years ago

(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.

	:Gijs (he/him) Assignee
	Comment 17 • 11 years ago

Upping per discussion on IRC.

	:Gijs (he/him) Assignee
	Comment 18 • 11 years ago

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

Try looks great. :-)

	Jared Wein [:jaws] (please needinfo? me)
	Comment 19 • 11 years ago

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?

	:Gijs (he/him) Assignee
	Comment 20 • 11 years ago

(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?

	:Gijs (he/him) Assignee
	Comment 21 • 11 years ago

And into the fray once more:

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

	Carsten Book [:Tomcat]
	Comment 22 • 11 years ago

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

	:Gijs (he/him) Assignee
	Comment 23 • 11 years ago

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

	:Gijs (he/him) Assignee
	Comment 24 • 11 years ago

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