Talos needs to enforce browser shutdown when testing add-on performance

RESOLVED WONTFIX

Status

defect
RESOLVED WONTFIX
8 years ago
8 years ago

People

(Reporter: gaubugzilla, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

8 years ago
I looked at the raw testing results for Adblock Plus and realized that they are completely bogus. Reason is the first-run page that Adblock Plus opens during profile initialization. That page has a beforeunload handler - so if it managed to load and initialize it will prevent the browser from closing. Whether this page loads before browser shutdown is triggered is really a matter of luck - in the latest round of tests Adblock Plus testing crashed on OS X and Leopard but not Windows 7 and Windows XP.
(Reporter)

Comment 1

8 years ago
It seems that other than Adblock Plus also NoScript, Web of Trust and BetterPrivacy are affected.
I'm not sure of a way to get around this without some means of disabling your first run page... talos needs to be able to close the browser reliably to continue testing.

Do you track fist load in some way that we could bluff it?
(In reply to comment #2)
> Do you track fist load in some way that we could bluff it?

Add-ons normally track the first run page using a preference. They usually have a boolean preference, but in the case of ABP, it's using pref "extensions.adblockplus.currentVersion", which is set to the version number. It was set to "1.3.6" on my last test.
(Reporter)

Comment 4

8 years ago
Adding a preference will do - but there are three more extensions where the first browser start times out (NoScript, BetterPrivacy, ImTranslator). Does Talos simply close the first-run page to quit the browser? That page can get chrome privileges, then it should be able to use nsIAppStartup.quit(eForceQuit).
goQuitApplication is used to shut down the browser after onload is fired.

We can kill the browser from the outside, the problem being that talos considers a browser that fails to close itself as frozen and thus busted.
If each of those addons could provide the pref/setting that they use to indicate that this is not first run it would be simple enough to place that in the talos configuration.
(In reply to comment #6)
> If each of those addons could provide the pref/setting that they use to
> indicate that this is not first run it would be simple enough to place that in
> the talos configuration.

What will the generic long term solution be?
Either we add each pref as a one off until we have a full list, or we create some sort of disable_addon_first_run that addon authors are encouraged to comply with.
(In reply to comment #8)
> Either we add each pref as a one off until we have a full list, or we create
> some sort of disable_addon_first_run that addon authors are encouraged to
> comply with.

that's bug 459965
(Reporter)

Comment 10

8 years ago
NoScript opens a first-run page on every update so you would need to adjust the value of that pref with each new version (not practicable) or add some more complicated logic (ugly).

I think the problem with goQuitApplication() is that it asks nicely first (via canQuitApplication()). This might be a good idea when testing the browser - but with add-ons I don't think that we want to ask nicely. It should ignore the result from quit-application-requested notification and quit nevertheless.
If we force quit out of the first run, will it then assume that every subsequent opening of the browser is first run?  I'd prefer not to have force quit as part of the standard for talos for all browser closing, if it just for the first initialization run then an argument could be made that it is part of the initial configuration stage.
(Reporter)

Comment 12

8 years ago
After looking into this a little more - nsIStartup.quit(eForceQuit) will not help with the modal beforeunload warning in the Adblock Plus first-run page, neither will it help with the modal dialog opened by BetterPrivacy. I wonder whether there is a proper way to shut down the application if modal dialogs are open (simply closing will do little good in case of BetterPrivacy, it demands a decision from the user and will open further modal dialogs). So I am not sure there is a good generic solution after all, shutting down externally will not save prefs.js.

Turned out that the reason NoScript tests time out is not its first-run page, it simply blocks scripts on startup_test.html.

In case of Adblock Plus fixing bug 647561 will do. For NoScript this pref works:

>   noscript.default: 'file:// about:blank about:credits addons.mozilla.org mozilla.net flashgot.net google.com gstatic.com googleapis.com securecode.com informaction.com yahoo.com yimg.com yahooapis.com maone.net noscript.net hotmail.com msn.com passport.com passport.net passportimages.com live.com js.wlxrs.com'

This is the usual value of that preference, only with file:// added. This way file:// is added to the default whitelist (replacing the whitelist is simpler but probably a bad idea since it might change performance).
The correct way to deal with this is through bug bug 459965 - talos will not be going in a direct where it doesn't allow the browser to shut down cleanly.

For the noscript issue please file a separate bug for updating the addon.config file to include the new pref.
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.