1111022 - Self-support v0: load self-support page in a hidden tab

Status: RESOLVED FIXED

(Firefox :: General, enhancement)

Description

[Benjamin Smedberg]

Implement the initial loading mechanism for self-support v0. See the user story for the technical spec.

Comment 1

[Gregg Lind (Fx Strategy and Insights - Shield - Heartbeat )]

I

Comment 2

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Creates an hidden, self support tab.

Comment 3

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8541455 [details] [diff] [review]
part 1 - Creates an hidden, self support tab.

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

::: browser/modules/SelfSupportBackend.jsm
@@ +39,5 @@
> +   * Initializes the self support backend.
> +   */
> +  init: function () {
> +    // Only allow to use SelfSupport if FHR is enabled.
> +    let fhrEnabled = Services.prefs.getBoolPref(FHR_ENABLED);

Those pref functions throw when there is no such pref.
Use Preferences.jsm instead and provide a sane default value.

@@ +47,5 @@
> +
> +    // Make sure UITour is enabled.
> +    let uiTourEnabled = Services.prefs.getBoolPref(UITOUR_ENABLED);
> +    if (!uiTourEnabled) {
> +      return;

This seems possibly ok for v0, but we will not limit selfsupport to only UI tour features.
We probably want to selectively enable features (e.g. heartbeat when UITour available).

@@ +51,5 @@
> +      return;
> +    }
> +
> +    // Check the preferences to see if we want this to be active.
> +    this._active = Services.prefs.getBoolPref(SELFSUPPORT_ENABLE_PREF);

Lets make it _enabled to match the pref name.

@@ +59,5 @@
> +
> +    this._delay = Services.prefs.getIntPref(SELFSUPPORT_DELAY_PREF);
> +
> +    Services.obs.addObserver(this, "sessionstore-windows-restored", false);
> +    Services.obs.addObserver(this, "quit-application-requested", false);

Maybe consider putting the topics into this.OBSERVERS and add/remove them via a loop or similar.

@@ +143,5 @@
> +      }
> +    }
> +
> +    // We've got the container.
> +    this._selfSupportWindow = bestContainerWindow;

Are we guaranteed that bestContainerWindow is non-null here? (I.e. that windowsEnumerator has at least one element?)
_loadSelfSupport() seems to assume that.

@@ +164,5 @@
> +
> +    let retryInterval = Services.prefs.getIntPref(SELFSUPPORT_RETRY_PREF);
> +
> +    // Add the new tab and hide it.
> +    this._selfSupportTab = this._selfSupportWindow.gBrowser.addTab(url);

What if _selfSupportWindow is null? E.g. because of the above comment or because _findSelfSupportTabContainer() threw?

@@ +175,5 @@
> +    // Define the progress listener to catch loading errors.
> +    let progressListener = {
> +      onLocationChange: function(aWebProgress, aRequest, aLocation, aFlags) {
> +        // If we land on an error page, try loading again after some time.
> +        if (aFlags & Ci.nsIWebProgressListener.LOCATION_CHANGE_ERROR_PAGE) {

Can we have some logging for code paths like this and also normal actions (like say "will selfsupport from url ...").
Log.jsm is helpful here.

Comment 4

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Creates an hidden, self support tab. (v2)

Thank you for the feedback! This patch addresses you concerns.

As for the UITour thing, you're right, we should probably be selectively enabling/disabling features based on what's enabled. Should I leave this for the next revision?

Comment 5

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8542172 [details] [diff] [review]
part 1 - Creates an hidden, self support tab. (v2)

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

::: browser/app/profile/firefox.js
@@ +1070,5 @@
> +pref("browser.selfsupport.delay", 5000);
> +// Enable the Self Support service.
> +pref("browser.selfsupport.enable", true);
> +// Dump the log to stdout? (if false, only prints to the browser console)
> +pref("browser.selfsupport.logdump", false);

".log.dump"/".log.level"?

::: browser/modules/SelfSupportBackend.jsm
@@ +12,5 @@
> +
> +Cu.import("resource://gre/modules/Log.jsm");
> +Cu.import("resource://gre/modules/Preferences.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource:///modules/sessionstore/SessionStore.jsm");

Looks unused.

@@ +31,5 @@
> +const FHR_ENABLED = "datareporting.healthreport.service.enabled";
> +// UITour status.
> +const UITOUR_ENABLED = "browser.uitour.enabled";
> +
> +const LOGGER_NAME = "browser.selfsupportbackend";

You can use camel-case for the name.

@@ +169,5 @@
> +      let pinnedTabCount = win.gBrowser._numPinnedTabs;
> +      let tabCount = win.gBrowser.tabs.length;
> +
> +      // That's the first window, let's not be too picky about it.
> +      if (bestContainerWindow === null) {

|if (!obj)| seems sufficient for these null-checks.

@@ +213,5 @@
> +   *        The window which will contain the new tab.
> +   */
> +  _loadSelfSupport: function(aWindow) {
> +    // Make sure we have a valid window container at this point.
> +    if (this._selfSupportWindow === null) {

|if (!obj)|?

@@ +218,5 @@
> +      this._logger.error("SelfSupport container is null.");
> +      return;
> +    }
> +
> +    // Fetch the Self Support URl from the preferences.

Nit: URL vs. URl.

@@ +222,5 @@
> +    // Fetch the Self Support URl from the preferences.
> +    let unformattedURL = this._prefs.get(URL_PREF, "https://some.nice.default");
> +    let url = Services.urlFormatter.formatURL(unformattedURL);
> +
> +    this._logger.config("Will SelfSupport from: " + url);

"Will load ..."

Comment 6

[Georg Fritzsche [:gfritzsche]]

(In reply to Alessio Placitelli [:Dexter] from comment #4)
> As for the UITour thing, you're right, we should probably be selectively
> enabling/disabling features based on what's enabled. Should I leave this for
> the next revision?

We can address this later, but its good to keep in mind that this won't always be limited to only heartbeat.

Comment 7

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Creates an hidden, self support tab. (v3)

This patch addresses all the previous comments, and adds the |waitForSelfSupportLoad| function which allows to wait for the self support page to load in tests.

Comment 8

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Creates an hidden, self support tab. (v4)

This patch fixes a bug which made |waitForSelfSupportLoad| resolve before page content was fully loaded.

Comment 9

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Adds some browser mochitests (WIP).

This patch adds browser mochitests to test for self-support page load and UI Tour API access.

SelfSupport is disabled by default in the test profile, otherwise every mochitest (even not SelfSupport ones) fails due to a tab being open when going to the next test (see: http://dxr.mozilla.org/mozilla-central/source/testing/mochitest/browser-test.js#216).

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8543231 [details] [diff] [review]
part 1 - Creates an hidden, self support tab. (v4)

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

Sorry if I'm repeating what was already said but I just had a cursory look at the patch...

I don't think we should add a hidden tab. This breaks tests as you already found out and it will break expectations and add-ons. The JSM should add a <browser> element to the hiddenWindow (like the CustomizationTabPreloader, ideally sharing some of that code) and load the self-support page in there. That way you can also make it a non-e10s <browser> which currently you can't control if you use a browser window.

How does the JSM expose the API? Cycling through all tabs and finding the one can't be the answer. Especially not if we're not using a tab :)

::: browser/app/profile/firefox.js
@@ +1066,5 @@
>  pref("browser.rights.override", true);
>  #endif
>  
> +// Load the self-support after this delay, at startup.
> +pref("browser.selfsupport.delay", 5000);

Seems overkill having this be a preference, a const should be enough.

@@ +1072,5 @@
> +pref("browser.selfsupport.enable", true);
> +// Dump the log to stdout? (if false, only prints to the browser console)
> +pref("browser.selfsupport.log.dump", false);
> +// The log level for the self support log branch.
> +pref("browser.selfsupport.log.level", "Warn");

We shouldn't have the logger "spam" the console by default. I'd rather think a pref to enable logging (disabled by default) would make more sense.

@@ +1074,5 @@
> +pref("browser.selfsupport.log.dump", false);
> +// The log level for the self support log branch.
> +pref("browser.selfsupport.log.level", "Warn");
> +// The retry interval if the self support URL fails to load.
> +pref("browser.selfsupport.retryInterval", 30000);

That should be a simple const too.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Can we use createWindowlessBrowser for this? We don't use it that much right now, and ran into some issues using it in bug 859672, but it seems ideal for this.

Comment 12

[Alessio Placitelli [:Dexter]]

(In reply to Tim Taubert [:ttaubert] from comment #10)
> Sorry if I'm repeating what was already said but I just had a cursory look
> at the patch...

Thanks for your input on this! ;) Also, thank you Gavin for your suggestion!

> I don't think we should add a hidden tab. This breaks tests as you already
> found out and it will break expectations and add-ons. The JSM should add a
> <browser> element to the hiddenWindow (like the CustomizationTabPreloader,
> ideally sharing some of that code) and load the self-support page in there.
> That way you can also make it a non-e10s <browser> which currently you can't
> control if you use a browser window.
> How does the JSM expose the API? Cycling through all tabs and finding the
> one can't be the answer. Especially not if we're not using a tab :)

Yes, using an hidden tab comes with some subtle problems. The UITour API is accessed by giving the proper privileges to the loaded page (i.e. "uitour") which will then make use of |CustomEvent| (see http://dxr.mozilla.org/mozilla-central/source/browser/modules/test/uitour.js#27) to interact with the API JSM (see http://dxr.mozilla.org/mozilla-central/source/browser/modules/UITour.jsm#329 ).

Access to this API allows, among other things, to show up the Heartbeat UI from bug 1111027. Would this be possible at all by using the |hiddenWindow| or |createWindowlessBrowser|?
 
> ::: browser/app/profile/firefox.js
> @@ +1066,5 @@
> >  pref("browser.rights.override", true);
> >  #endif
> >  
> > +// Load the self-support after this delay, at startup.
> > +pref("browser.selfsupport.delay", 5000);
> 
> Seems overkill having this be a preference, a const should be enough.

Thanks for pointing this out! I left this as a preference in order to change its default value from the test profile to remove the 5 seconds delay.

> We shouldn't have the logger "spam" the console by default. I'd rather think
> a pref to enable logging (disabled by default) would make more sense.

Good point, I'll change that!

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #11)
> Can we use createWindowlessBrowser for this? We don't use it that much right
> now, and ran into some issues using it in bug 859672, but it seems ideal for
> this.

Oha, is that a new thing? :) That sounds perfect indeed.

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Alessio Placitelli [:Dexter] from comment #12)
> Yes, using an hidden tab comes with some subtle problems. The UITour API is
> accessed by giving the proper privileges to the loaded page (i.e. "uitour")
> which will then make use of |CustomEvent| (see
> http://dxr.mozilla.org/mozilla-central/source/browser/modules/test/uitour.
> js#27) to interact with the API JSM (see
> http://dxr.mozilla.org/mozilla-central/source/browser/modules/UITour.jsm#329
> ).

The permission is given only to the URI itself, the string, not a specific docShell or browser. That should work.

> Access to this API allows, among other things, to show up the Heartbeat UI
> from bug 1111027. Would this be possible at all by using the |hiddenWindow|
> or |createWindowlessBrowser|?

The UITour API itself is implemented by an event listener in a content script. If you call browser.messageManager.loadFrameScript("...content-UITour.js") then also your browser in the hidden window will have the correct frame script that sends messages. The UITour part in the parent process listens for messages on the global message manager and will thus receive those. That should work as well then.

Comment 15

[Alessio Placitelli [:Dexter]]

I see, thank you Tim for taking time digging into this!

My main concern with |createWindowlessBrowser| is the usage of the UI API. As of now, the UITour API assumes new UI bits (e.g. info panel, notifications) should be shown in the Window owning the loaded document [1].

Given the specs mentioned in the User Story, we want the tab to open in the window with most pinned tabs/tabs, because we want the UI bits to show there. Without using the hidden tab approach, if I got that correctly, this would mean changing the behaviour of the UITour API to show things in different windows.

[1] - http://dxr.mozilla.org/mozilla-central/source/browser/modules/UITour.jsm#331

Comment 16

[Georg Fritzsche [:gfritzsche]]

(In reply to Alessio Placitelli [:Dexter] from comment #15)
> My main concern with |createWindowlessBrowser| is the usage of the UI API.
> As of now, the UITour API assumes new UI bits (e.g. info panel,
> notifications) should be shown in the Window owning the loaded document [1].

We should be able to change that behavior without too much effort, so we can instead e.g. specify a window, right?

The simple heuristic in the user story was to find a "main window" to put the hidden tab in. We probably should show the UI tour in the active/focused/foreground window?

Comment 17

[Alessio Placitelli [:Dexter]]

Attachment: SelfSupport in hidden browser (no tab alternative) - WIP

This patch contains an alternative SelfSupport implementation, based on both Tim's and Gavin's suggestions.

It does not make use of tabs but, instead, uses a <browser> attached to an |hiddenDOMWindow|. This should help us with the problem discussed in the comments above.

Unfortunately, this introduces an issue with the way the UITour API works right now. The UITour makes precise assumptions [1] about the |window| used to display its stuff. Moreover, |gBrowser| [2] does not seem to be defined on this type of window. To make use of this version of the SelfSupport backend, we should be able to select which window should be the target of our UITour calls.

Matthew, does this make sense at all to you? Is it realistic to modify UITour for this purpose? 

[1] - https://dxr.mozilla.org/mozilla-central/source/browser/components/uitour/UITour.jsm#331
[2] - https://dxr.mozilla.org/mozilla-central/source/browser/components/uitour/UITour.jsm#332

Comment 18

[Gregg Lind (Fx Strategy and Insights - Shield - Heartbeat )]

Given 

1. the amount of work to make UITour instantiate with getMostRecentWindow()
2. how little of the Tour heartbeat needs (none!)

I wonder if it makes more sense to make this part of self-repair-ui (or similar).

Tour seems to give the benefits of 
1. already defined CustomEvents
2. some blessings around permissions?  (which, will those be kept, if there is no URI for Services.perms?)

Is this much much different than showNotificationBar()?

Comment 19

[Benjamin Smedberg]

That does make more sense, once we have a security story for exposing the support API. That's blocked on bug 1073191. UITour was the simple solution.

Comment 20

[:Gavin Sharp [email: [e-mail]]]

We should generally avoid using Services.appShell.hiddenDOMWindow where possible. Did createWindowlessBrowser not work? Comment 16 should address your concern in comment 15.

Comment 21

[:Gavin Sharp [email: [e-mail]]]

(In reply to Gregg Lind (User Advocacy - Heartbeat - Test Pilot) from comment #18)
> Given 
> 
> 1. the amount of work to make UITour instantiate with getMostRecentWindow()

I don't understand this - there shouldn't be anything complicated here. Happy to help if there are specific obstacles/concerns.

Comment 22

[Gregg Lind (Fx Strategy and Insights - Shield - Heartbeat )]

How about:  that it requires *any work* and puts the Tour at risk for its actual purpose, of giving the tour :).  Otherwise, no objection.

Comment 23

[Alessio Placitelli [:Dexter]]

Attachment: part 3 - Changes UITour.jsm to work with windowless browsers.

This patch changes UITour to allow its usage from documents in windowless/hidden browsers.

Comment 24

[Alessio Placitelli [:Dexter]]

Attachment: SelfSupport in windowless browser (no tab, createWindowlessBrowser alternative) - WIP

Thanks Gavin and everybody else for your feedback!

This is another patch I've been working on, which makes use of |createWindowlessBrowser| instead of using |Services.appShell.hiddenDOMWindow|. 

I'm probably doing something wrong with |createWindowlessBrowser|, but I can't seem to find a way to catch UITour calls, as [1] does not get called. I feel like it could have something to do with |chromeEventHandler| being null for the windowless browser.

[1] - https://dxr.mozilla.org/mozilla-central/source/browser/components/uitour/content-UITour.js#12

Comment 25

[Matthew N. [:MattN]]

Comment on attachment 8545862 [details] [diff] [review]
part 3 - Changes UITour.jsm to work with windowless browsers.

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

Please write a test for this to make sure we don't regress UITour working in a windowless browser in the future.

::: browser/modules/UITour.jsm
@@ +331,5 @@
>      let window = browser.ownerDocument.defaultView;
> +
> +    // Windowless browsers do not define |gBrowser| so we catch this case.
> +    let isWindowlessBrowser = (typeof window.gBrowser == "undefined");
> +    if (isWindowlessBrowser) {

Nit: I think having the variable more accurately describe what it represents would be helpful. Something like `windowHasTabs`. Continuing to mention windowless browsers as an example in the comment is fine but I would slightly re-word it after this change.

@@ +336,5 @@
> +      // When using windowless browsers we don't have a valid |window|. If that's the case,
> +      // use the most recent window as a target for UITour functions (see Bug 1111022).
> +      let wm = Components.classes["@mozilla.org/appshell/window-mediator;1"]
> +               .getService(Components.interfaces.nsIWindowMediator);
> +      window = wm.getMostRecentWindow("navigator:browser");

Nit: You can use Services.wm.getMostRecentWindow("navigator:browser");

@@ +343,1 @@
>      let tab = window.gBrowser.getTabForBrowser(browser);

Could you add a comment above this line mentioning that `tab` can be null such as when a tab is getting detached or when running in a windowless browser?

@@ +661,5 @@
>        }
>      }
>  
> +    // Non-e10s. See bug 1089000. Also, windowless browsers do not belong to a tab.
> +    if (!window.gMultiProcessBrowser && !isWindowlessBrowser) {

Is it really `!tab` that should be checked here?

Comment 26

[Matthew N. [:MattN]]

Comment on attachment 8545351 [details] [diff] [review]
SelfSupport in hidden browser (no tab alternative) - WIP

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

I think that the changes required to UITour in the other patch are fine.

::: browser/modules/SelfSupportBackend.jsm
@@ +56,5 @@
> +   * Initializes the self support backend.
> +   */
> +  init: function () {
> +    this._prefs = new Preferences(PREF_BRANCH);
> +    let otherPrefs = new Preferences();

Nit: it's rare to use pref branches in new code nowadays since it makes it harder to find where a preference is used via code-search

@@ +96,5 @@
> +   */
> +  uninit: function () {
> +    // This service was not active in the first place.
> +    if (!this._enabled) {
> +      this._logger.debug("SelfService was disabled, nothing to shut down.");

Nit: "SelfService" is mentioned in a few of the logging statements instead of "SelfSupport"

@@ +116,5 @@
> +        this._logger.debug("SelfService is about to spawn the hidden browser.");
> +
> +        setTimeout(() => {
> +            this._loadSelfSupport();
> +          }, STARTUP_DELAY);

Nit: setTimeout(this._loadSelfSupport.bind(this), STARTUP_DELAY);

@@ +186,5 @@
> +    this._logger.config("Will load SelfSupport from: " + url);
> +
> +    // Allow the URL to use the UITour.
> +    let pageURI = Services.io.newURI(url, null, null);
> +    Services.perms.add(pageURI, "uitour", Services.perms.ALLOW_ACTION);

This permission will get removed if a user uses Clear Recent History (CRH) and checks Site Preferences. I would prefer we have the domain whitelisted by default in default_permissions.

Comment 27

[Gregg Lind (Fx Strategy and Insights - Shield - Heartbeat )]

(Debugging nits:  prefs-based things that don't have prefs listeners mean that to make changes needs a restart to propogate.  In particular the log level, the pageURI etc.  I worry that whitelisting the domain will have the same issue.)

Comment 28

[Gregg Lind (Fx Strategy and Insights - Shield - Heartbeat )]

Not having the prefs be responsive makes it hard to write jetpack addons that change those prefs without killing and restarting the UITour service.  

Drawbacks to listeners:  what should happen if we change page url?  Should it existing self-repair and restart it?

Comment 29

[Alessio Placitelli [:Dexter]]

Attachment: part 3 - Changes UITour.jsm to work with windowless browsers (v2).

Thank you Matthew for your review! This patch implements the modification you requested.

Comment 30

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - SelfSupport in hidden browser (hiddenDOMWindow)

This patch addresses the suggestions by MattN.

Good point for the permission granting line, I'll remove that once we get the URL whitelisted.

Also, Gregg raises a good point with the debugging nit. I'll deal with this once we decide which version to go with (|hiddenDOMWindow| or |createWindowlessBrowser|, the latter not currently working).

Comment 31

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Adds some browser mochitests (v2).

Comment 32

[Alessio Placitelli [:Dexter]]

Attachment: part 4 - Adds a test for UITour and Windowless Browsers

Comment 33

[Alessio Placitelli [:Dexter]]

After a bit of delay and number of tests (thanks Tim!), I can't seem to find a way to make |createWindowlessBrowser| work for our case.

Even though we're able to open the SelfSupport page within the browser, I couldn't find a way to get get messages pass from content to chrome. There are two scenarios:

- |createWindowlessBrowser|, using a "chrome" type browser (argument to |true|): we can get a messageManager, can load a frame script, but no message get caught by the content script.
- using a "content" type browser (argument to |false|): we can't get a messageManager (not directly nor indirectly from other interfaces).

I cross checked this issue with :ttaubert (again, thanks for your time Tim!), as suggested. Gavin, should move on and continue with the |hiddenDOMWindow|?

It seems to be the browser returned from this function allows to do something, but not quite everything.

Comment 34

[:Gavin Sharp [email: [e-mail]]]

You know your time constraints better than I do, but I would prefer pushing on bug 1123697 just a bit harder still.

Comment 35

[Matthew N. [:MattN]]

Comment on attachment 8546769 [details] [diff] [review]
part 3 - Changes UITour.jsm to work with windowless browsers (v2).

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

Sorry for the delay… I was trying to do some cleanup of UITour to simplify your patches.

::: browser/components/uitour/UITour.jsm
@@ +332,5 @@
> +
> +    // Does the window have tabs? We need to make sure since windowless browsers do
> +    // not have tabs.
> +    let windowHasTabs = (typeof window.gBrowser != "undefined");
> +    if (!windowHasTabs) {

I would be fine with just checking (!window.gBrowser). I don't think you need the comment above and below so I would just remove the comment below.

@@ +367,5 @@
>      window.gBrowser.tabContainer.addEventListener("TabSelect", this);
>  
>      if (!window.gMultiProcessBrowser) { // Non-e10s. See bug 1089000.
>        contentDocument = browser.contentWindow.document;
> +      if (!tab && windowHasTabs) {

This hunk and the one below are no longer needed after cleanup in bug 1089000 and bug 1110602.

Comment 36

[Matthew N. [:MattN]]

Comment on attachment 8546769 [details] [diff] [review]
part 3 - Changes UITour.jsm to work with windowless browsers (v2).

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

I missed a case in my teardown patch where a tab was assumed to exist. You should add this to your patch:

         let tab = aWindow.gBrowser.getTabForBrowser(browser);
-        tab.removeEventListener("TabClose", this);
+        if (tab) {
+          tab.removeEventListener("TabClose", this);
+        }

Comment 37

[Matthew N. [:MattN]]

Comment on attachment 8551476 [details] [diff] [review]
part 4 - Adds a test for UITour and Windowless Browsers

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

::: browser/components/uitour/test/browser.ini
@@ +22,5 @@
>  [browser_UITour_modalDialog.js]
>  run-if = os == "mac" # modal dialog disabling only working on OS X
>  skip-if = e10s # Bug 941428 - UITour.jsm not e10s friendly
> +[browser_UITour_no_tabs.js]
> +skip-if = e10s # Bug 941428 - UITour.jsm not e10s friendly.

Don't forgot to re-sort this after renaming the file.

Also, depending on the solution we end up with, this test may be able to run with e10s (at least it does with the hidden window) and I really would like at least on UITour test running in e10s.

::: browser/components/uitour/test/browser_UITour_no_tabs.js
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */

Nit: tests are normally licensed as Public Domain and they are now Public Domain by default so you don't need this comment.

Nit: we don't need the "_UITour" in the test file names now that we recently moved to our own component directory.

Comment 38

[Tim Taubert [:ttaubert] (inactive)]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #34)
> You know your time constraints better than I do, but I would prefer pushing
> on bug 1123697 just a bit harder still.

Just posted a comment in that bug on our findings from today.

Another more philosophical question: we were told that the "windowless browser" is actually quite similar to the hiddenWindow. We are supposed to load a XUL frame and then create a <browser> inside it if we expect/need all the nice things we are used to, including message managers and frame scripts.

Even if we fix the one thing that prevents us from using it currently the approach is actually super similar to what we did in the BrowserNewTabPreloader and still do in the CustomizationTabPreloader. Just instead of using the hiddenWindow we use an "anonymous" hidden window.

I am not sure if there really are any advantages to using the anonymous one. The hiddenWindow is shared by multiple features so they might in theory conflict. It's also not very helpful that the hiddenWindow is XUL on Mac and XHTML on Win/Linux. Other than that though I can't think of any really convincing arguments. Even for the "anonymous" hidden window we would need quite a lot of boilerplate code that would then ideally land in WindowlessBrowser.jsm or the like.

Comment 39

[Alessio Placitelli [:Dexter]]

Attachment: part 3 - Changes UITour.jsm to work with windowless browsers (v3).

This patch removes the unneeded hunks from the previous patch.

Comment 40

[Alessio Placitelli [:Dexter]]

Attachment: part 4 - Adds a test for UITour and Windowless Browsers (v2)

Changed the copyright notice and allowed the test to run when e10s is enabled.

Comment 41

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - SelfSupport in hidden browser (hiddenDOMWindow).

This implements the logging prefs changes requested.

Comment 42

[Alessio Placitelli [:Dexter]]

As per IRC talk with Gavin and Gregg, I will be using |hiddenDOMWindow| for this initial release. When |createWindowlessBrowser| gets in an usable status, I'll follow up and eventually change the implementation to use it.

Comment 43

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8554512 [details] [diff] [review]
part 1 - SelfSupport in hidden browser (hiddenDOMWindow).

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

::: browser/app/profile/firefox.js
@@ +1027,5 @@
>  pref("browser.rights.override", true);
>  #endif
>  
> +// Enable the Self Support service.
> +pref("browser.selfsupport.enable", true);

Should be .enabled for consistency.
This should be a hidden pref per the user story.

@@ +1033,5 @@
> +pref("browser.selfsupport.log.dump", false);
> +// The log level for the self support log branch.
> +pref("browser.selfsupport.log.level", "Warn");
> +// The self support tab.
> +pref("browser.selfsupport.url", "https://www.selfsupport.placeholder");

Will this get set to something proper?
Why not just leave it unset in the mean-time for "don't do anything"?
The user story says it should be a hidden pref anyway.

::: browser/base/content/browser.js
@@ +22,5 @@
>                                    "resource://gre/modules/Task.jsm");
>  XPCOMUtils.defineLazyModuleGetter(this, "CharsetMenu",
>                                    "resource://gre/modules/CharsetMenu.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "SelfSupportBackend",
> +                                  "resource://gre/modules/SelfSupportBackend.jsm");

Unused?

::: browser/modules/SelfSupportBackend.jsm
@@ +17,5 @@
> +Cu.import("resource://gre/modules/Timer.jsm");
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +
> +// Enables or disables the Self Support.
> +const ENABLE_PREF = "browser.selfsupport.enable";

.enabled

@@ +27,5 @@
> +const UITOUR_ENABLED = "browser.uitour.enabled";
> +
> +// Controls the interval at which the self support page tries to reload in case of
> +// errors.
> +const RETRY_INTERVAL = 30000;

Whats the unit? Maybe name it RETRY_INTERVAL_MS/_SEC/...?

@@ +29,5 @@
> +// Controls the interval at which the self support page tries to reload in case of
> +// errors.
> +const RETRY_INTERVAL = 30000;
> +// The delay after which to load the self-support, at startup.
> +const STARTUP_DELAY = 5000;

Unit?

@@ +43,5 @@
> +let gLogAppenderDump = null;
> +
> +this.SelfSupportBackend = {
> +  _browser: null,
> +  _deferredSupportPageLoad: null,

Document what these are for?

@@ +45,5 @@
> +this.SelfSupportBackend = {
> +  _browser: null,
> +  _deferredSupportPageLoad: null,
> +  _enabled: false,
> +  _frame: null,

Document this.

@@ +51,5 @@
> +  _OBSERVERS: [
> +    "sessionstore-windows-restored",
> +    "quit-application-requested"
> +  ],
> +  _reloadTimerId: null,

Document this.

@@ +96,5 @@
> +
> +  /**
> +   * Shut down the self support backend, if active.
> +   */
> +  uninit: function () {

Don't we need to cancel self support page loads etc. and wait for that?

@@ +119,5 @@
> +    this._log.trace("observe - Topic " + aTopic);
> +
> +    switch (aTopic) {
> +      case "sessionstore-windows-restored": {
> +        setTimeout(this._loadSelfSupport.bind(this), STARTUP_DELAY);

We need to be able to cancel this on uninit().

@@ +122,5 @@
> +      case "sessionstore-windows-restored": {
> +        setTimeout(this._loadSelfSupport.bind(this), STARTUP_DELAY);
> +        break;
> +      }
> +      case "quit-application-requested": {

Why do we need this when we have uninit()?

@@ +146,5 @@
> +
> +  /**
> +   * Return a promise which is resolved when SelfSupport page is correctly loaded.
> +   */
> +  waitForSelfSupportLoad: function() {

This is exposed for tests?
Can we document it as such?

@@ +153,5 @@
> +
> +  /**
> +   * Gets the hidden browser.
> +   */
> +  getBrowser: function() {

Why do we need to expose this? Testing?

@@ +160,5 @@
> +
> +  /**
> +   * Configure the logger based on the preferences.
> +   */
> +  _configureLogging: function() {

Side-note: I think i have to start to repeat what others said before - we need to add a common implementation for this :(
That might make a good mentored bug if we could get it specd out properly.

@@ +189,5 @@
> +  /**
> +   * Create a frame in the |hiddenDOMWindow| to host our |browser|, then load the SelfSupport
> +   * page in it.
> +   **/
> +  _makeHiddenBrowser: function(url) {

I don't really know about these, but i think ttaubert has good context here.
Please ask Tim to review _makeHiddenBrowser & _loadSelfSupport.

@@ +196,5 @@
> +
> +    // Create a HTML iframe with a chrome URL, then this can host the browser.
> +    this._frame = hiddenDoc.createElementNS(HTML_NS, "iframe");
> +    this._frame.setAttribute("src", "chrome://global/content/mozilla.xhtml");
> +    this._frame.addEventListener("load", function onLoad() {

If you use an arrow function you don't need to .bind().
This only resolves on "load". Will you always get "load"? Shutdown, odd load failures, ...

@@ +226,5 @@
> +    this._log.config("Will load SelfSupport from: " + url);
> +
> +    // Create the hidden browser.
> +    let browserPromise = this._makeHiddenBrowser(url);
> +    browserPromise.then(() => {

This seems to assume _makeHiddenBrowser() always succeeds?

@@ +241,5 @@
> +          if (aFlags & Ci.nsIWebProgressListener.LOCATION_CHANGE_ERROR_PAGE) {
> +            self._log.warn("There was a problem fetching the SelfSupport URL.");
> +
> +            // Reload the page after the retry interval expires.
> +            self._reloadTimerId = setTimeout(() =>{

Nit: missing space between =>{

Comment 44

[Georg Fritzsche [:gfritzsche]]

Re the user story and when to check/load selfsupport content:
> * This content should be loaded at or very soon after startup (see below for more details).

Should we also check again periodically to cover long-running sessions?

Comment 45

[Benjamin Smedberg]

Not in this iteration. We'll revisit that later.

Comment 46

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - SelfSupport in hidden browser (hiddenDOMWindow) - v2

Tim, would you mind reviewing the _makeHiddenBrowser & _loadSelfSupport bits?

Do you know of any way to catch IFRAME creation errors/browser creation errors? In other words, any idea on how to catch _makeHiddenBrowser errors?

Comment 47

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8555305 [details] [diff] [review]
part 1 - SelfSupport in hidden browser (hiddenDOMWindow) - v2

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

::: browser/modules/SelfSupportBackend.jsm
@@ +11,5 @@
> +const Ci = Components.interfaces;
> +
> +Cu.import("resource://gre/modules/Log.jsm");
> +Cu.import("resource://gre/modules/Preferences.jsm");
> +Cu.import("resource://gre/modules/Promise.jsm");

We shouldn't need to import Promise.jsm (once we got rid of the legacy Promise.defer() code below).

@@ +43,5 @@
> +const XUL_NS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
> +
> +let gLogAppenderDump = null;
> +
> +this.SelfSupportBackend = {

This currently exposes all "private/hidden" properties. We should split this into a public and private API.

@@ +91,5 @@
> +      return;
> +    }
> +
> +    // Make sure we have a valid promise to wait for the self support page load.
> +    this._deferredSupportPageLoad = Promise.defer();

Promise.defer() is deprecated, please use |new Promise()|.

@@ +144,5 @@
> +  },
> +
> +  /**
> +   * Only used in tests. Return a promise which is resolved when SelfSupport page is correctly loaded.
> +   */

The API shouldn't expose test functionality. Maybe we should rethink whether the test needs to be that fine-grained and know all about the implementation? We have been using waitForCondition() to wait conditions we can't directly check in other tests.

@@ +151,5 @@
> +  },
> +
> +  /**
> +   * Only used in tests. Gets the hidden browser.
> +   */

Same here about test functionality in the API.

@@ +159,5 @@
> +
> +  /**
> +   * Configure the logger based on the preferences.
> +   */
> +  _configureLogging: function() {

I'm not a huge fun of logging/debugging code mixed with everything else. This seems a little too much, a huge function only to set up logging in case we need it? Using the browser debugger seems the better approach here if we want to find out what's going wrong. This is 2015 :)

@@ +189,5 @@
> +   * Create a frame in the |hiddenDOMWindow| to host our |browser|, then load the SelfSupport
> +   * page in it.
> +   **/
> +  _makeHiddenBrowser: function(aURL) {
> +    let deferred = Promise.defer();

Promise.defer() is deprecated, please use |new Promise()|.

@@ +195,5 @@
> +
> +    // Create a HTML iframe with a chrome URL, then this can host the browser.
> +    this._frame = hiddenDoc.createElementNS(HTML_NS, "iframe");
> +    this._frame.setAttribute("src", "chrome://global/content/mozilla.xhtml");
> +    this._frame.addEventListener("load", function onLoad() {

Can we reuse the HiddenFrame code from CustomizationTabPreloader.jsm? Put this in a HiddenFrame.jsm maybe?

@@ +205,5 @@
> +      this._browser.setAttribute("src", aURL);
> +
> +      this._frame.contentDocument.documentElement.appendChild(this._browser);
> +      deferred.resolve();
> +    }.bind(this), true);

.bind(this) returns a complete new function. Calling removeEventListener(onLoad) won't work then.

@@ +228,5 @@
> +    let browserPromise = this._makeHiddenBrowser(url);
> +    browserPromise.then(() => {
> +      // Load UITour frame script.
> +      this._browser.messageManager.loadFrameScript(
> +        "chrome://browser/content/content-UITour.js", false);

Nit: that URL should go in a const at the top.

@@ +252,5 @@
> +      };
> +
> +      // Make sure to notify when the page correctly loads.
> +      this._browser.addEventListener("load", function loadListener() {
> +        this._log.debug("_loadSelfSupport - Page correctly loaded.");

Listening for the "load" event AND using a progress listener seems overkill. If we want to handle network errors as well we should simply use the progress listener and wait for (STATE_DOCUMENT | STATE_STOP) that would (I think) then handle both situations.

@@ +256,5 @@
> +        this._log.debug("_loadSelfSupport - Page correctly loaded.");
> +        this._browser.removeEventListener("load", loadListener, true);
> +        this._browser.removeProgressListener(progressListener);
> +        this._deferredSupportPageLoad.resolve();
> +      }.bind(this), true);

Same here for .bind(this).

Comment 48

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - SelfSupport in hidden browser (hiddenDOMWindow) - v3

Comment 49

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Adds some browser mochitests (v2).

Updated the tests to work with the latest revision of SelfSupport backend. These tests only cover the following things:

- SelfSupport page loading.
- UITour API usage from the hidden self support page.

Comment 50

[Alessio Placitelli [:Dexter]]

Thank you for your time and the effort reviewing this, Tim!

(In reply to Tim Taubert [:ttaubert] from comment #47)
> Comment on attachment 8555305 [details] [diff] [review]
> part 1 - SelfSupport in hidden browser (hiddenDOMWindow) - v2
> 
> Review of attachment 8555305 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +144,5 @@
> > +  },
> > +
> > +  /**
> > +   * Only used in tests. Return a promise which is resolved when SelfSupport page is correctly loaded.
> > +   */
> 
> The API shouldn't expose test functionality. Maybe we should rethink whether
> the test needs to be that fine-grained and know all about the
> implementation? We have been using waitForCondition() to wait conditions we
> can't directly check in other tests.

That's, indeed, a good point. I don't like this solution either, but it seemed the most practical solution.

> @@ +159,5 @@
> > +
> > +  /**
> > +   * Configure the logger based on the preferences.
> > +   */
> > +  _configureLogging: function() {
> 
> I'm not a huge fun of logging/debugging code mixed with everything else.
> This seems a little too much, a huge function only to set up logging in case
> we need it? Using the browser debugger seems the better approach here if we
> want to find out what's going wrong. This is 2015 :)

I'd like to keep logging code in SelfSupportBackend, since, IMHO, it makes it easier to debug and understand what's going on when dealing with async stuff. Also, I think that logging is handy for modules executing before the browser debugger is available (as it's the case for SelfSupport Backend). Or is there a way to debug a JSM module before Firefox starts?

> @@ +195,5 @@
> > +
> > +    // Create a HTML iframe with a chrome URL, then this can host the browser.
> > +    this._frame = hiddenDoc.createElementNS(HTML_NS, "iframe");
> > +    this._frame.setAttribute("src", "chrome://global/content/mozilla.xhtml");
> > +    this._frame.addEventListener("load", function onLoad() {
> 
> Can we reuse the HiddenFrame code from CustomizationTabPreloader.jsm? Put
> this in a HiddenFrame.jsm maybe?

Good point! I've filed bug 1128500 for that.

Comment 51

[Georg Fritzsche [:gfritzsche]]

(In reply to Alessio Placitelli [:Dexter] from comment #50)
> > @@ +159,5 @@
> > > +
> > > +  /**
> > > +   * Configure the logger based on the preferences.
> > > +   */
> > > +  _configureLogging: function() {
> > 
> > I'm not a huge fun of logging/debugging code mixed with everything else.
> > This seems a little too much, a huge function only to set up logging in case
> > we need it? Using the browser debugger seems the better approach here if we
> > want to find out what's going wrong. This is 2015 :)
> 
> I'd like to keep logging code in SelfSupportBackend, since, IMHO, it makes
> it easier to debug and understand what's going on when dealing with async
> stuff. Also, I think that logging is handy for modules executing before the
> browser debugger is available (as it's the case for SelfSupport Backend). Or
> is there a way to debug a JSM module before Firefox starts?

Also having at least basic logging of operations is useful for QA, quickly diagnosing things and to get info in test logs.

Comment 52

[Georg Fritzsche [:gfritzsche]]

(In reply to Tim Taubert [:ttaubert] from comment #47)
> I'm not a huge fun of logging/debugging code mixed with everything else.
> This seems a little too much, a huge function only to set up logging in case
> we need it? Using the browser debugger seems the better approach here if we
> want to find out what's going wrong. This is 2015 :)

I think the biggest issue here is that preffable configs should be in Log.jsm at some point.
It's more a question of who can find the time to add it.

Comment 53

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - SelfSupport in hidden browser (hiddenDOMWindow) - v4

Compared to v3, this patch removes the test functions from the API.

Comment 54

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Adds some browser mochitests (v3).

The tests have been changed to work without the test API methods.

Comment 55

[Gregg Lind (Fx Strategy and Insights - Shield - Heartbeat )]

(getting a .rej on the modules/moz.build for Part 1)

Comment 56

[Alessio Placitelli [:Dexter]]

(In reply to Gregg Lind (User Advocacy - Heartbeat - Test Pilot) from comment #55)
> (getting a .rej on the modules/moz.build for Part 1)

moz.build has probably changed since the version my patch is based on. If you want to test the patch, in this case, you could manually apply the changes in .rej to the moz.build file (should simply be adding the SelfSupportBackend.jsm line). I'll rebase the patches once the reviews come in!

Comment 57

[Georg Fritzsche [:gfritzsche]]

(In reply to Alessio Placitelli [:Dexter] from comment #53)
> Created attachment 8558634 [details] [diff] [review]
> part 1 - SelfSupport in hidden browser (hiddenDOMWindow) - v4
> 
> Compared to v3, this patch removes the test functions from the API.

And this is now split between SelfSupportBackend and SelfSupportBackendInternal.
I assume you talked with Tim about that - can you please recap this here?

Comment 58

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8558634 [details] [diff] [review]
part 1 - SelfSupport in hidden browser (hiddenDOMWindow) - v4

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

::: browser/modules/SelfSupportBackend.jsm
@@ +74,5 @@
> +
> +  /**
> +   * Initializes the self support backend.
> +   * @param waitForRestore If true, waits for the "sessionstore-windows-restored" message
> +   *        before triggering page load. Otherwise, starts the loading timer immediately.

I assume this is for testing only?
If it is, we should document that fact.

Comment 59

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8558636 [details] [diff] [review]
part 2 - Adds some browser mochitests (v3).

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

This doesn't reset the prefs properly. We should ideally do this so we still clean them up on test failures, e.g. using registerCleanupFunction.

I think this is still missing test coverage for these points from the user story:
* The content in the hidden page should be able to close itself by firing a custom UITour event.
* The content in the hidden page should be able to show itself, probably by custom UITour event.
Is that coming in a later patch?

::: browser/modules/test/browser_SelfSupportBackend.js
@@ +33,5 @@
> +  }
> +  return null;
> +}
> +
> +function waitSelfSupportLoad(aURL, aSuccess) {

Lets just make this promiseSelfSupportLoad(aURL) and return a promise.

@@ +35,5 @@
> +}
> +
> +function waitSelfSupportLoad(aURL, aSuccess) {
> +  waitForConditionPromise(() => {
> +    let browser = findSelfSupportBrowser(aURL);

|() => !!findSelfSupportBrowser(aURL)|?

@@ +48,5 @@
> + * Test that the self support page correctly loads.
> + */
> +add_task(function* test_loads() {
> +  // Make sure the self support backend is initially disabled.
> +  is(Preferences.get(PREF_SELFSUPPORT_ENABLED, false), false, "SelfSupport should be initially disabled.");

Please use Assert.jsm functions in new test code.

@@ +52,5 @@
> +  is(Preferences.get(PREF_SELFSUPPORT_ENABLED, false), false, "SelfSupport should be initially disabled.");
> +
> +  // Enable the SelfSupport backend.
> +  Preferences.set(PREF_SELFSUPPORT_ENABLED, true);
> +  is(Preferences.get(PREF_SELFSUPPORT_ENABLED, false), true, "SelfSupport should now be enabled.");

This seems redundant, we just set the pref here.

@@ +71,5 @@
> +  SelfSupportBackend.uninit();
> +
> +  // Disable the SelfSupport backend.
> +  Preferences.set(PREF_SELFSUPPORT_ENABLED, false);
> +  is(Preferences.get(PREF_SELFSUPPORT_ENABLED, false), false, "SelfSupport should now be disabled.");

Redundant.

@@ +83,5 @@
> +  is(Preferences.get(PREF_SELFSUPPORT_ENABLED, false), false, "SelfSupport should be initially disabled.");
> +
> +  // Make sure UITour is enabled.
> +  Preferences.set(PREF_UITOUR_ENABLED, true);
> +  is(Preferences.get(PREF_UITOUR_ENABLED, false), true, "UITour should be enabled.");

Redundant.

@@ +87,5 @@
> +  is(Preferences.get(PREF_UITOUR_ENABLED, false), true, "UITour should be enabled.");
> +
> +  // Enable the SelfSupport backend.
> +  Preferences.set(PREF_SELFSUPPORT_ENABLED, true);
> +  is(Preferences.get(PREF_SELFSUPPORT_ENABLED, false), true, "SelfSupport should now be enabled.");

Redundant.

@@ +104,5 @@
> +  yield deferred.promise;
> +
> +  // Find the SelfSupport tab.
> +  let selfSupportBrowser = findSelfSupportBrowser(pageURL);
> +  isnot(selfSupportBrowser, null, "SelfSupport browser must exist and not be null.");

Nit: "and not be null" seems redundant.

Comment 60

[Alessio Placitelli [:Dexter]]

> And this is now split between SelfSupportBackend and
> SelfSupportBackendInternal.
> I assume you talked with Tim about that - can you please recap this here?

Yes, I talked to Tim about the public API/private API split. I've decided to keep only |init|/|uninit| as public.

|waitForRestore| is currently only used in tests.

Comment 61

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - SelfSupport in hidden browser (hiddenDOMWindow) - v5

Updated to use HiddenFrame instances. Also, marked |waitForRestore| as used in tests.

Comment 62

[Alessio Placitelli [:Dexter]]

Attachment: part 5 - Change UITour to allow self support to close itself.

This patch introduces a new UITour message, needed to cover the user story requirement of Self Support Backend being able to close itself.

After discussing this with Benjamin, since for v0 the page won't need to show itself, we will cover the other requirement in a followup bug.

Comment 63

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Adds browser mochitests (v4).

Thank you Georg for your review. This patch makes the test use Assert.jsm, removes redundant messages and adds a test to cover this user story requirement:

* The content in the hidden page should be able to close itself by firing a custom UITour event.

Comment 64

[Benjamin Smedberg]

Do you really want to do this via UITour instead of making window.close() work? Standard DOM APIs for the win!

Comment 65

[Alessio Placitelli [:Dexter]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #64)
> Do you really want to do this via UITour instead of making window.close()
> work? Standard DOM APIs for the win!

Good point! I just tried this approach: from the content, I called |window.close()| (if that's what you meant!).

This closed the whole |hiddenDOMWindow|, making it unavailable for further uses.

Another approach could be to traverse the hierarchy hiddenDOMWindow->document[x]->IFrame->browser->content backwards up to [x] and remove the IFrame. I'm not sure that's possible from |content|. If it were possible, since SelfSupportBackend.jsm holds a reference to both the IFrame and the browser, I don't know if it would get GC'd.

Comment 66

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Georg Fritzsche [:gfritzsche] from comment #52)
> (In reply to Tim Taubert [:ttaubert] from comment #47)
> > I'm not a huge fun of logging/debugging code mixed with everything else.
> > This seems a little too much, a huge function only to set up logging in case
> > we need it? Using the browser debugger seems the better approach here if we
> > want to find out what's going wrong. This is 2015 :)
> 
> I think the biggest issue here is that preffable configs should be in
> Log.jsm at some point.
> It's more a question of who can find the time to add it.

Fair point, doesn't seem as overkill then anymore I guess.

Comment 67

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8559150 [details] [diff] [review]
part 1 - SelfSupport in hidden browser (hiddenDOMWindow) - v5

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

::: browser/modules/SelfSupportBackend.jsm
@@ +11,5 @@
> +const Ci = Components.interfaces;
> +
> +Cu.import("resource://gre/modules/Log.jsm");
> +Cu.import("resource://gre/modules/Preferences.jsm");
> +Cu.import("resource://gre/modules/PromiseUtils.jsm");

Unused.

@@ +39,5 @@
> +const PREF_BRANCH_LOG = "browser.selfsupport.log.";
> +const PREF_LOG_LEVEL = PREF_BRANCH_LOG + "level";
> +const PREF_LOG_DUMP = PREF_BRANCH_LOG + "dump";
> +
> +const SELFSUPPORT_DEFAULT_URL = "https://some.nice.default";

Should that be a pref?

@@ +75,5 @@
> +  /**
> +   * Initializes the self support backend.
> +   * @param waitForRestore If true, waits for the "sessionstore-windows-restored" message
> +   *        before triggering page load. Otherwise, starts the loading timer immediately.
> +   *        Used it tests.

We should get rid of that parameter. When running a single test on your machine that might add a little delay but in reality, whenever running tests on our CI infrastructure, the module will be loaded by the time the test is run. If not we'll just wait a little longer.

@@ +123,5 @@
> +    Preferences.ignore(PREF_BRANCH_LOG, this._configureLogging, this);
> +
> +    // This service was not active in the first place.
> +    if (!this._enabled) {
> +      this._log.debug("uninit - SelfSupport was disabled, nothing to shut down.");

Seems not worth keeping the |_enabled| flag around. uninit() will work fine even if there's nothing uninitialize.

@@ +139,5 @@
> +      this._frame = null;
> +    }
> +
> +    // Dispose of the hidden browser.
> +    if (this._browser !== null) {

Should destroy the browser before the frame.

@@ +162,5 @@
> +  /**
> +   * Configure the logger based on the preferences.
> +   */
> +  _configureLogging: function() {
> +    if (!this._log) {

This function is called from init() only once. So |this._log| should always be falsy/null?

@@ +181,5 @@
> +        gLogAppenderDump = new Log.DumpAppender(new Log.BasicFormatter());
> +        this._log.addAppender(gLogAppenderDump);
> +      } else {
> +        this._log.removeAppender(gLogAppenderDump);
> +        gLogAppenderDump = null;

Seems like we'd never reach this when _configureLogging() is called only once.

@@ +190,5 @@
> +  /**
> +   * Triggers the self support loading mechanism.
> +   */
> +  _triggerPageLoad: function () {
> +    this._delayedLoadTimerId = setTimeout(this._loadSelfSupport.bind(this), STARTUP_DELAY_MS);

Can just move this into observe() when we get rid of the |waitForRestore| argument above.

@@ +198,5 @@
> +   * Create an hidden frame to host our |browser|, then load the SelfSupport page in it.
> +   * @param aURL The URL to load in the browser.
> +   */
> +  _makeHiddenBrowser: function(aURL) {
> +    let promise = new Promise((resolve, reject) => {

Nit: reject is unused

@@ +200,5 @@
> +   */
> +  _makeHiddenBrowser: function(aURL) {
> +    let promise = new Promise((resolve, reject) => {
> +      this._frame = new HiddenFrame();
> +      this._frame.get().then(aFrame => {

You don't need an extra promise, you can just do:

this._frame = ...
return this._frame.get().then(frame => {
  // ...
  doc.documentElement.appendChild( ...
});

@@ +232,5 @@
> +    // Create the hidden browser.
> +    let browserPromise = this._makeHiddenBrowser(url);
> +    browserPromise.then(() => {
> +      // Load UITour frame script.
> +      this._browser.messageManager.loadFrameScript(UITOUR_FRAME_SCRIPT, false);

Does this work?? With allowDelayedLoad=false the frame needs to be ready, which it might be... If you see intermittent failures or can't get it to work it might be worth switching that to true.

@@ +247,5 @@
> +            // Reload the page after the retry interval expires.
> +            self._reloadTimerId = setTimeout(() => {
> +              self._log.debug("_loadSelfSupport - Reloading SelfSupport URL in the hidden browser.");
> +              self._browser.reload(Ci.nsIWebNavigation.LOAD_FLAGS_NONE);
> +            }, RETRY_INTERVAL_MS);

With the ProgressListener as its own class you can do:

aWebProgress.DOMWindow.setTimeout(() => aWebProgress.DOMWindow.location.reload(), RETRY_INTERVAL_MS);

We don't really need to keep track of the timeout id, when the browser shuts down the window will go away and we don't need to cancel the timeout.

It would be good if the ProgressListener implemented a max. number of retries. If I'm on a plane I wouldn't want Firefox to try loading this page every 30s. Alternatively (or additionally) we could also increase the interval with every retry by some factor.

@@ +269,5 @@
> +      let webFlags = Ci.nsIWebProgress.NOTIFY_STATE_WINDOW |
> +                     Ci.nsIWebProgress.NOTIFY_STATE_REQUEST |
> +                     Ci.nsIWebProgress.NOTIFY_LOCATION;
> +
> +      this._browser.addProgressListener(progressListener, webFlags);

I think it would be nicer having the progress listener in a separate object:

let progressListener = new ProgressListener(this._browser);
this._browser.addProgressListener(progressListener, flags);

Comment 68

[Benjamin Smedberg]

> Good point! I just tried this approach: from the content, I called
> |window.close()| (if that's what you meant!).
> 
> This closed the whole |hiddenDOMWindow|, making it unavailable for further
> uses.

That's not good! I suspect you're running into some edge case at http://hg.mozilla.org/mozilla-central/annotate/58ce6051edf5/dom/base/nsGlobalWindow.cpp#l8622

You can probably handle this by adding a "DOMWindowClose" event listener and handling the event yourself as you describe, by removing the iframe. See

http://hg.mozilla.org/mozilla-central/annotate/58ce6051edf5/dom/base/nsGlobalWindow.cpp#l8520
and the standard tabbed-browsing handler: http://hg.mozilla.org/mozilla-central/annotate/58ce6051edf5/browser/base/content/tabbrowser.xml#l3525

Comment 69

[Alessio Placitelli [:Dexter]]

> @@ +39,5 @@
> > +const PREF_BRANCH_LOG = "browser.selfsupport.log.";
> > +const PREF_LOG_LEVEL = PREF_BRANCH_LOG + "level";
> > +const PREF_LOG_DUMP = PREF_BRANCH_LOG + "dump";
> > +
> > +const SELFSUPPORT_DEFAULT_URL = "https://some.nice.default";
> 
> Should that be a pref?

It is, |PREF_URL|. This constant is the default value in case that pref has no value set. Now that server-side bugs have landed, default value can be changed to http://self-repair.mozilla.org/%LOCALE%/repair .

> @@ +75,5 @@
> > +  /**
> > +   * Initializes the self support backend.
> > +   * @param waitForRestore If true, waits for the "sessionstore-windows-restored" message
> > +   *        before triggering page load. Otherwise, starts the loading timer immediately.
> > +   *        Used it tests.
> 
> We should get rid of that parameter. When running a single test on your
> machine that might add a little delay but in reality, whenever running tests
> on our CI infrastructure, the module will be loaded by the time the test is
> run. If not we'll just wait a little longer.

Unfortunately, as far as I can tell, the "sessionstore-windows-restored" event is emitted only once when the browser starts in browser tests. That means that only the first test, when running the test file as standalone, gets the event. The following tests do not, as the browser is already started and everything is already restored. When we exposed |observe|, I trigger loading by doing something similar to https://dxr.mozilla.org/mozilla-central/source/toolkit/components/crashmonitor/test/unit/test_register.js#.

Am I missing something/is there any other way around this?

> @@ +139,5 @@
> > +      this._frame = null;
> > +    }
> > +
> > +    // Dispose of the hidden browser.
> > +    if (this._browser !== null) {
> 
> Should destroy the browser before the frame.

Should this be fixed in https://dxr.mozilla.org/mozilla-central/source/browser/modules/CustomizationTabPreloader.jsm#63 as well?

> @@ +162,5 @@
> > +  /**
> > +   * Configure the logger based on the preferences.
> > +   */
> > +  _configureLogging: function() {
> > +    if (!this._log) {
> 
> This function is called from init() only once. So |this._log| should always
> be falsy/null?

Only the first time |_configureLogging| is called (from init() as you mentioned). But this can also be called when logging preferences change because of |Preferences.observe|. If that happens, we don't want to append another ConsoleAppender. I should probably document it better though.

> @@ +181,5 @@
> > +        gLogAppenderDump = new Log.DumpAppender(new Log.BasicFormatter());
> > +        this._log.addAppender(gLogAppenderDump);
> > +      } else {
> > +        this._log.removeAppender(gLogAppenderDump);
> > +        gLogAppenderDump = null;
> 
> Seems like we'd never reach this when _configureLogging() is called only
> once.

Same as above, since the pref |PREF_LOG_DUMP| can change and trigger this.

> @@ +190,5 @@
> > +  /**
> > +   * Triggers the self support loading mechanism.
> > +   */
> > +  _triggerPageLoad: function () {
> > +    this._delayedLoadTimerId = setTimeout(this._loadSelfSupport.bind(this), STARTUP_DELAY_MS);
> 
> Can just move this into observe() when we get rid of the |waitForRestore|
> argument above.

Sure, it was like that before. But we need to cope with the initialisation problem for tests.

> @@ +232,5 @@
> > +    // Create the hidden browser.
> > +    let browserPromise = this._makeHiddenBrowser(url);
> > +    browserPromise.then(() => {
> > +      // Load UITour frame script.
> > +      this._browser.messageManager.loadFrameScript(UITOUR_FRAME_SCRIPT, false);
> 
> Does this work?? With allowDelayedLoad=false the frame needs to be ready,
> which it might be... If you see intermittent failures or can't get it to
> work it might be worth switching that to true.

Yes, this definitely works. But if |true| is more correct/makes it more solid, there's no reason why this should be |false|.

Comment 70

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Alessio Placitelli [:Dexter] from comment #69)
> > > +const SELFSUPPORT_DEFAULT_URL = "https://some.nice.default";
> > 
> > Should that be a pref?
> 
> It is, |PREF_URL|. This constant is the default value in case that pref has
> no value set. Now that server-side bugs have landed, default value can be
> changed to http://self-repair.mozilla.org/%LOCALE%/repair .

We should probably not have a default value for that. We ship a pref that sets the URL and if the pref is empty we should probably just bail out as if the whole module wasn't enabled.

> > @@ +75,5 @@
> > > +  /**
> > > +   * Initializes the self support backend.
> > > +   * @param waitForRestore If true, waits for the "sessionstore-windows-restored" message
> > > +   *        before triggering page load. Otherwise, starts the loading timer immediately.
> > > +   *        Used it tests.
> > 
> > We should get rid of that parameter. When running a single test on your
> > machine that might add a little delay but in reality, whenever running tests
> > on our CI infrastructure, the module will be loaded by the time the test is
> > run. If not we'll just wait a little longer.
> 
> Unfortunately, as far as I can tell, the "sessionstore-windows-restored"
> event is emitted only once when the browser starts in browser tests. That
> means that only the first test, when running the test file as standalone,
> gets the event. The following tests do not, as the browser is already
> started and everything is already restored.

Your test shouldn't listen for any of those notifications, it should simply check if the API frame is already loaded. If not then wait for the load, otherwise continue with the test.

> > @@ +139,5 @@
> > > +      this._frame = null;
> > > +    }
> > > +
> > > +    // Dispose of the hidden browser.
> > > +    if (this._browser !== null) {
> > 
> > Should destroy the browser before the frame.
> 
> Should this be fixed in
> https://dxr.mozilla.org/mozilla-central/source/browser/modules/
> CustomizationTabPreloader.jsm#63 as well?

Ummm... yeah. Didn't catch this when I reviewed the other patch.

> > @@ +162,5 @@
> > > +  /**
> > > +   * Configure the logger based on the preferences.
> > > +   */
> > > +  _configureLogging: function() {
> > > +    if (!this._log) {
> > 
> > This function is called from init() only once. So |this._log| should always
> > be falsy/null?
> 
> Only the first time |_configureLogging| is called (from init() as you
> mentioned). But this can also be called when logging preferences change
> because of |Preferences.observe|.

My bad, missed the Preferences.observe() line when looking for call sites. Please ignore :)

> > > +      this._browser.messageManager.loadFrameScript(UITOUR_FRAME_SCRIPT, false);
> > 
> > Does this work?? With allowDelayedLoad=false the frame needs to be ready,
> > which it might be... If you see intermittent failures or can't get it to
> > work it might be worth switching that to true.
> 
> Yes, this definitely works. But if |true| is more correct/makes it more
> solid, there's no reason why this should be |false|.

I think it would be more solid... I'm sure it doesn't hurt at least :)

Comment 71

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Georg Fritzsche [:gfritzsche] from comment #43)
> > +// Enable the Self Support service.
> > +pref("browser.selfsupport.enable", true);
> 
> Should be .enabled for consistency.
> This should be a hidden pref per the user story.

I assume the user story talks about a pref that's not exposed in the UI. If you want a "hidden pref" with its default value in code, then that just means it's a hard-coded value that can be overriden with a pref. We should probably just go with a normal pref as we did for FxAccounts and other modules.

Comment 72

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - SelfSupport in hidden browser (hiddenDOMWindow) - v6

Thanks Benjamin for pointing me to the right direction. Catching DOMWindowClose definitely helps and make |window.close()| work!

Changes in this patch:

- Removes testing functions.
- Implemented a load attempts limit.
- Retry timeout increases with the number of attempts.
- Allows SelfSupport page to close itself using |window.close()| without closing the hiddenDOMWindow.

URL default value is still available in the code.
TheProgressListener was not refactored in a separate object as |aWebProgress.DOMWindow.setTimeout| does not seem to work when the page is an error page in |onLocationChange|. It works fine in |onStateChange| when the page is correctly loaded.

Comment 73

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Adds browser mochitests (v5).

This patch changes the tests so that we don't need to expose testing functions from SelfSupportBackend.

I've dealt with this by manually calling |Services.obs.notifyObservers(null, "sessionstore-windows-restored", null);| to trigger the loading process. I understand other listeners are receiving this notification as well, but it does not seem to break anything on my system (and I saw a similar approach used in other tests as well). I'm open to other suggestions though.

Since SelfSupportBackend is *always* enabled in tests, we shut it down here in order to test the loading process.

Comment 74

[Georg Fritzsche [:gfritzsche]]

(In reply to Tim Taubert [:ttaubert] from comment #71)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #43)
> > > +// Enable the Self Support service.
> > > +pref("browser.selfsupport.enable", true);
> > 
> > Should be .enabled for consistency.
> > This should be a hidden pref per the user story.
> 
> I assume the user story talks about a pref that's not exposed in the UI. If
> you want a "hidden pref" with its default value in code, then that just
> means it's a hard-coded value that can be overriden with a pref. We should
> probably just go with a normal pref as we did for FxAccounts and other
> modules.

I was assuming the latter version. But that is probably just something for Benjamin to clarify then if he has a strong opinion on it.

Comment 75

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8560868 [details] [diff] [review]
part 1 - SelfSupport in hidden browser (hiddenDOMWindow) - v6

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

::: browser/modules/SelfSupportBackend.jsm
@@ +120,5 @@
> +    // Dispose of the hidden browser.
> +    if (this._browser !== null) {
> +      let doc = this._browser.contentDocument;
> +      if (doc) {
> +        doc.defaultView.removeEventListener("DOMWindowClose", this, true);

this._browser.contentWindow?

@@ +230,5 @@
> +      // We need to watch for load errors as well and, in case, try to reload
> +      // the self support page.
> +      let self = this;
> +      let progressListener = {
> +        onLocationChange: function(aWebProgress, aRequest, aLocation, aFlags) {

This is still inside _loadSelfSupport() but should really be a separate object. If this really needs logging then the logger should be separate too.

Comment 76

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - SelfSupport in hidden browser (hiddenDOMWindow) - v7

Thank you for your time reviewing this, Tim.

I've put the progress listener in a separate object and, after talking to Benjamin, the URL pref in firefox.js.

Comment 77

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8562131 [details] [diff] [review]
part 1 - SelfSupport in hidden browser (hiddenDOMWindow) - v7

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

::: browser/modules/SelfSupportBackend.jsm
@@ +230,5 @@
> +      const webFlags = Ci.nsIWebProgress.NOTIFY_STATE_WINDOW |
> +                       Ci.nsIWebProgress.NOTIFY_STATE_REQUEST |
> +                       Ci.nsIWebProgress.NOTIFY_LOCATION;
> +
> +      let progressListener = new ProgressListener(() => {

Good start! Can we please move the callbacks to separate functions of |SelfSupportBackendInternal|?

@@ +238,5 @@
> +        // Increase the number of attempts and bail out if we failed too many times.
> +        this._loadAttempts++;
> +        if (this._loadAttempts > MAX_RETRIES) {
> +          this._log.info("_loadSelfSupport - Too many failed load attempts. Giving up.");
> +          this.uninit();

Should remove the progress listener in that case as well.

@@ +277,5 @@
> +
> +ProgressListener.prototype = {
> +  onLocationChange: function(aWebProgress, aRequest, aLocation, aFlags) {
> +    if (aFlags & Ci.nsIWebProgressListener.LOCATION_CHANGE_ERROR_PAGE) {
> +      this._loadErrorCallback();

I think it would be much nicer if the ProgressListener would handle the retries. It would try to reload the page and only after MAX_RETRIES is hit call the loadErrorCallback.

Comment 78

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - SelfSupport in hidden browser (hiddenDOMWindow) - v8

Thanks for the quick review!

Changes:

- Callbacks as separate functions;
- Retries are handled by the progress listener;

Comment 79

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8560869 [details] [diff] [review]
part 2 - Adds browser mochitests (v5).

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

::: browser/modules/test/browser_SelfSupportBackend.js
@@ +11,5 @@
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "SelfSupportBackend",
> +                                  "resource:///modules/SelfSupportBackend.jsm");
> +
> +const PREF_SELFSUPPORT_ENABLED = "browser.selfsupport.enable";

"enabled", not "enable".

@@ +30,5 @@
> +  for (let frame of frames) {
> +    try {
> +      let browser = frame.contentDocument.getElementById("win").querySelectorAll('browser')[0];
> +      let url = browser.getAttribute("src");
> +      if (url == aURL && browser.contentDocument.readyState === "complete") {

Once we found the right browser, can't we properly wait for the load to complete?
See e.g. promiseTabLoadEvent() et al in browser/base/content/test/general/head.js.

@@ +50,5 @@
> + *          loaded.
> + */
> +function promiseSelfSupportLoad(aURL) {
> +  return waitForConditionPromise(() => !!findSelfSupportBrowser(aURL), "Timeout",
> +                                 TEST_WAIT_RETRIES);

The message could be more descriptive.

@@ +63,5 @@
> + *          be found anymore.
> + */
> +function promiseSelfSupportClose(aURL) {
> +  return waitForConditionPromise(() => !!!findSelfSupportBrowser(aURL), "Timeout",
> +                                 TEST_WAIT_RETRIES);

Why triple-negate?
The message could be more descriptive.

@@ +69,5 @@
> +
> +/**
> + * Prepare the test environment.
> + */
> +add_task(function* test_loads() {

The function needs renaming.

@@ +84,5 @@
> +  const TEST_PAGE_URL = getRootDirectory(gTestPath) + "uitour.html";
> +
> +  // Make sure the self support backend is initially disabled.
> +  Assert.ok(!Preferences.get(PREF_SELFSUPPORT_ENABLED, false),
> +            "SelfSupport should be initially disabled.");

We just set that pref a few lines earlier, we don't need to check it here.
Also, we don't really do anything before set the pref to true. Why do we set it to false in the first place?

@@ +103,5 @@
> +  yield promiseSelfSupportLoad(TEST_PAGE_URL);
> +
> +  // Shutdown and disable SelfSupport backend
> +  SelfSupportBackend.uninit();
> +  Preferences.set(PREF_SELFSUPPORT_ENABLED, false);

I don't think we always need to set the pref to false.
We should just take care that we reset it properly on test end (i.e. registerCleanupFunction).

@@ +114,5 @@
> +  const TEST_PAGE_URL = getRootDirectory(gTestPath) + "uitour.html";
> +
> +  // Make sure the self support backend is initially disabled.
> +  Assert.ok(!Preferences.get(PREF_SELFSUPPORT_ENABLED, false),
> +            "SelfSupport should be initially disabled.");

Do we need this?

@@ +131,5 @@
> +  Services.obs.notifyObservers(null, "sessionstore-windows-restored", null);
> +
> +  // Wait for the SelfSupport page to load.
> +  info("Waiting for the SelfSupport local page to load.");
> +  yield promiseSelfSupportLoad(TEST_PAGE_URL);

Up until here this looks nearly the same as the last test function.
Maybe we can just merge them?
Maybe merge in test_canCloseItself() too?

@@ +203,5 @@
> +  // doesn't create any problem.
> +  SelfSupportBackend.uninit();
> +});
> +
> +registerCleanupFunction(() => {

This is a little hidden here.
Lets just move that into the function that prepares the test environment.

Comment 80

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Adds browser mochitests (v6).

Thank you for your time reviewing this Georg. This patch merges the tests, removes the redundant checks and changes |promiseSelfSupportLoad|.

Comment 81

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8562233 [details] [diff] [review]
part 1 - SelfSupport in hidden browser (hiddenDOMWindow) - v8

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

Perfetto.

::: browser/modules/SelfSupportBackend.jsm
@@ +252,5 @@
> +                       Ci.nsIWebProgress.NOTIFY_STATE_REQUEST |
> +                       Ci.nsIWebProgress.NOTIFY_LOCATION;
> +
> +      this._progressListener = new ProgressListener(() => this._pageLoadErrorCallback(),
> +                                                    () => this._pageSuccessCallback());

Yay, this looks a lot better now. Thanks for your persistence :)

Comment 82

[Alessio Placitelli [:Dexter]]

(In reply to Tim Taubert [:ttaubert] from comment #81)
> Comment on attachment 8562233 [details] [diff] [review]
> part 1 - SelfSupport in hidden browser (hiddenDOMWindow) - v8
> 
> Review of attachment 8562233 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Perfetto.
> [...]
> Yay, this looks a lot better now. Thanks for your persistence :)

\o/ :-D Thank you for your patience on this!

Comment 83

[Alessio Placitelli [:Dexter]]

The current implementation of the SelfSupport backend makes some attempts (5) to load the support page in case of errors. Each time it waits a bit longer before attempting a reload. After the maximum number of attempts is reached, we bail out.

Both Tim and Gregg had some points about why bailing out is a good thing.

If our servers are temporarily down, we might bail out without trying to load self support again before Firefox restarts. Do we care about this case?

Comment 84

[Benjamin Smedberg]

No.

Comment 85

[Benjamin Smedberg]

Although it probably makes sense to add a telemetry probe to measure both the failure to load and the fact that in the end we bailed out.

Comment 86

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8562623 [details] [diff] [review]
part 2 - Adds browser mochitests (v6).

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

Thanks, this looks good. Only minor notes below.
I assume we have test coverage for the content showing itself via UITour in other tests?

::: browser/modules/test/browser_SelfSupportBackend.js
@@ +9,5 @@
> +let Preferences = Cu.import("resource://gre/modules/Preferences.jsm", {}).Preferences;
> +let PromiseUtils = Cu.import("resource://gre/modules/PromiseUtils.jsm", {}).PromiseUtils;
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "SelfSupportBackend",
> +                                  "resource:///modules/SelfSupportBackend.jsm");

Nit: It's a test and we use that module anyway, so we don't need to load it lazily.

@@ +99,5 @@
> +
> +  registerCleanupFunction(() => {
> +    Preferences.reset(PREF_SELFSUPPORT_ENABLED);
> +    Preferences.reset(PREF_SELFSUPPORT_URL);
> +    Preferences.reset(PREF_UITOUR_ENABLED);

Testing prefs are set via user_pref(), so we would lose their value here.
You should reset those to their value before this test.

@@ +120,5 @@
> +  yield promiseSelfSupportLoad(TEST_PAGE_URL);
> +
> +  // Find the SelfSupport browser.
> +  let selfSupportBrowser = findSelfSupportBrowser(TEST_PAGE_URL);
> +  Assert.ok(!!selfSupportBrowser, "SelfSupport browser must exist.");

If you make promiseSelfSupportLoad() return the browser, you can skip findSelfSupportBrowser().

Comment 87

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8562233 [details] [diff] [review]
part 1 - SelfSupport in hidden browser (hiddenDOMWindow) - v8

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

::: browser/modules/SelfSupportBackend.jsm
@@ +38,5 @@
> +const PREF_BRANCH_LOG = "browser.selfsupport.log.";
> +const PREF_LOG_LEVEL = PREF_BRANCH_LOG + "level";
> +const PREF_LOG_DUMP = PREF_BRANCH_LOG + "dump";
> +
> +const MAX_RETRIES = 5;

Suggestion: Could move this up to RETRY_INTERVAL_MS.

@@ +104,5 @@
> +  /**
> +   * Shut down the self support backend, if active.
> +   */
> +  uninit: function () {
> +    Preferences.ignore(PREF_BRANCH_LOG, this._configureLogging, this);

this._log.trace("shutdown")?

Comment 88

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - SelfSupport in hidden browser (hiddenDOMWindow) - v9

Thanks Georg, I've taken care of the nits here.

Comment 89

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Adds browser mochitests (v7).

Thanks for your time reviewing this. Preferences are now being set back to their values correctly and |promiseSelfSupportLoad| is returning a browser.

Comment 90

[Gregg Lind (Fx Strategy and Insights - Shield - Heartbeat )]

(is this ready or not?)

Comment 91

[Alessio Placitelli [:Dexter]]

(In reply to Gregg Lind (User Advocacy - Heartbeat - Test Pilot) from comment #90)
> (is this ready or not?)

Not yet, as I'm dealing with test bustages and need to land bug 1126756 first.

Comment 92

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Adds browser mochitests (v8).

Fixes the test failing on the try server.

Comment 93

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - SelfSupport in hidden browser (hiddenDOMWindow) - v9

Updated the commit message.

Comment 94

[Alessio Placitelli [:Dexter]]

Attachment: part 3 - Changes UITour.jsm to work with windowless browsers (v3).

Updated the commit message.

Comment 95

[Alessio Placitelli [:Dexter]]

Attachment: part 4 - Adds a test for UITour and Windowless Browsers (v3)

Updated the commit message, removed the wrong ".bind(this)".

Comment 96

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Adds browser mochitests (v8).

Updated the commit message.

Comment 97

[Alessio Placitelli [:Dexter]]

Try run (along with bug 1126756): https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec6d22c0ce68

@ttaubert: there's mochitest-oth test which is failing, "test_docload.html", with the following message:

153 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/events/test_docload.html | Different amount of expected children of [' no node info ', role: app root, name: 'Firefox', address: [xpconnect wrapped nsIAccessible]]. - got 3, expected 2

I saw in [1] you had some problems with this test and the hiddenWindow before. Do you have any clue on what's going on here?

[1] - http://hg.mozilla.org/mozilla-central/filelog/09f4968d5f42/accessible/tests/mochitest/events/test_docload.html

Comment 98

[Tim Taubert [:ttaubert] (inactive)]

Looks like you're hitting what I fixed in bug 794041 and reverted in bug 1077652 (Part 0012). We probably need to leave this in? Why doesn't the CustomizationTabPreloader hit this?

The a11y tests are awfully fragile... And it seems that the a11y team opposes bug 1103899 so we can only resort to fixing tests whenever we introduce something like that in the hidden window.

Comment 99

[Tim Taubert [:ttaubert] (inactive)]

Bug 879741 might also be of interest...

Comment 100

[Tim Taubert [:ttaubert] (inactive)]

Alessio, can you run the a11y test locally and see if it breaks (it should)?

And then apply the inverse of the following patch:
https://bug1077652.bugzilla.mozilla.org/attachment.cgi?id=8527600

And see if it still breaks? Thanks!

Comment 101

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Tim Taubert [:ttaubert] from comment #100)
> Alessio, can you run the a11y test locally and see if it breaks (it should)?

This can be done with:

mach mochitest-a11y accessible/tests/mochitest/events/test_docload.html

Comment 102

[Matthew N. [:MattN]]

Comment on attachment 8564967 [details] [diff] [review]
part 4 - Adds a test for UITour and Windowless Browsers (v3)

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

::: browser/components/uitour/test/browser_no_tabs.js
@@ +6,5 @@
> +const HTML_NS = "http://www.w3.org/1999/xhtml";
> +const XUL_NS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
> +
> +/**
> + * Create a frame in the |hiddenDOMWindow| to host a |browser|, then load the an URL in the

Typo: "the an"

Comment 103

[Alessio Placitelli [:Dexter]]

Thanks Matthew, I'll attach the updated and rebased patch later today.

Tim, as mentioned over IRC, I wasn't able to reproduce the issue locally (not on Windows, nor in an Ubuntu VM). I've pushed to try and the problem was still there.

I've applied your patch and it seems to be gone: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a23672acccd

Comment 104

[Alessio Placitelli [:Dexter]]

Attachment: part 5 - Fixes test_docload.html a11y test failing on try servers.

This patch fixes the a11y test failing on the try server when using hidden windows.

Comment 105

[Alessio Placitelli [:Dexter]]

Attachment: part 5 - Fixes test_docload.html a11y test failing on try servers.

Comment 106

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Adds browser mochitests (v9)

Fixed a typo and cleanup order of the browser/frame.

Comment 107

[Alessio Placitelli [:Dexter]]

Attachment: part 3 - Changes UITour.jsm to work with windowless browsers (v4)

Rebased patch and removed useless hunks due to bug 1126756.

Comment 108

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - SelfSupport in hidden browser (hiddenDOMWindow) - v10

Rebased the patch.

Comment 109

[Alessio Placitelli [:Dexter]]

Attachment: part 4 - Adds a test for UITour and Windowless Browsers (v4)

Fixed Typo.

Comment 110

[alexander :surkov (:asurkov)]

How this one correlates to bug 794041?

Comment 111

[Tim Taubert [:ttaubert] (inactive)]

(In reply to alexander :surkov from comment #110)
> How this one correlates to bug 794041?

It does the same thing, putting a <browser> in the hidden window. And a11y sadly still doesn't ignore stuff in the hidden window or from background tabs (inactive docShells).

Comment 112

[alexander :surkov (:asurkov)]

so some summary from those bugs: this events/test_docload.html patch goes from bug 794041 which was reverted in bug 1077652. And we have open bug 879741 to figure out what we should do with hidden windows in a11y.

Comment 113

[Georg Fritzsche [:gfritzsche]]

Can this land now or does it need another try run?

Comment 114

[Alessio Placitelli [:Dexter]]

I've pushed to try again, better to be safe.

Comment 115

[Alessio Placitelli [:Dexter]]

I've retriggered the failing tests, which appear to be intermittent issue not related to this patch (all is green now): https://treeherder.mozilla.org/#/jobs?repo=try&revision=27085e0b758b

Comment 116

[Ryan VanderMeulen [:RyanVM]]

Doesn't apply cleanly to inbound. Please rebase.

Comment 117

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - SelfSupport in hidden browser (hiddenDOMWindow) - v10

Rebased the patch.

Comment 118

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/236b975a2ca0
https://hg.mozilla.org/integration/fx-team/rev/c99674acf5d6
https://hg.mozilla.org/integration/fx-team/rev/2b12485b132e
https://hg.mozilla.org/integration/fx-team/rev/3e69b38f7d2e
https://hg.mozilla.org/integration/fx-team/rev/84faeab69114

Comment 119

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/236b975a2ca0
https://hg.mozilla.org/mozilla-central/rev/c99674acf5d6
https://hg.mozilla.org/mozilla-central/rev/2b12485b132e
https://hg.mozilla.org/mozilla-central/rev/3e69b38f7d2e
https://hg.mozilla.org/mozilla-central/rev/84faeab69114

Comment 120

[Gregg Lind (Fx Strategy and Insights - Shield - Heartbeat )]

(Is this going to uplift?  Otherwise 37 release notes might need editing.)

Comment 121

[Lawrence Mandel [:lmandel] (use needinfo)]

If this is needed for 37, please file a Beta approval request. At this point I would prefer that we take the change in Beta 2.

Comment 122

[Alessio Placitelli [:Dexter]]

Yes, I think we could aim to 37 Beta 2 uplift, as long as MattN agrees on the risk of uplifting bug 1126756 as well.

Comment 123

[Matthew N. [:MattN]]

(In reply to Alessio Placitelli [:Dexter] from comment #122)
> Yes, I think we could aim to 37 Beta 2 uplift, as long as MattN agrees on
> the risk of uplifting bug 1126756 as well.

I'm fine with uplifting that. Feel free to do the requests.

Comment 124

[Alessio Placitelli [:Dexter]]

Comment on attachment 8567616 [details] [diff] [review]
part 1 - SelfSupport in hidden browser (hiddenDOMWindow) - v10

We would like to uplift this to beta to make Firefox 37 Beta 2. This should be uplifted together with the following bugs (order matters):

* 1128500 (introduces the HiddenFrame object)
* 1126756 (allows UITour to cleanup properly)
* 1128564 (whitelist self-support URL to use UITour)
* 1111022 (this bug)

Please note that all of the patches in this bug need to be applied in the correct order shown in the patch file name (part 1, 2, 3, ...).

[Feature/regressing bug #]:
Needed for heartbeat / selfsupport v0, bug 1111016

[User impact if declined]:
Heartbeat and self-support will not be available for the user.

[Describe test coverage new/current, TreeHerder]: This bug introduces test coverage for self-support usage. A new UITour test is also introduced to cover UITour being used from windowless browsers/hidden windows.
[Risks and why]: Low risk, isolated to self support not being able to work. Can be easily disabled through an hidden preference.
[String/UUID change made/needed]: No

Comment 125

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8567616 [details] [diff] [review]
part 1 - SelfSupport in hidden browser (hiddenDOMWindow) - v10

Code change is isolated to Heartbeat, has tests, and has been on m-v for 5 days. Taking the remaining required Heartbeat changes in Beta 2. Beta+

Comment 126

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8567032 [details] [diff] [review]
part 2 - Adds browser mochitests (v9)

Test only changes. Beta+

Comment 127

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/41fd4a4df8ac
https://hg.mozilla.org/releases/mozilla-beta/rev/e7a0f9cd3482
https://hg.mozilla.org/releases/mozilla-beta/rev/f8c1586998ef
https://hg.mozilla.org/releases/mozilla-beta/rev/9b32fd0a9844
https://hg.mozilla.org/releases/mozilla-beta/rev/87d76aead804