995041 - Fix browser_aboutHome.js to not touch the outside network and re-enable it on all trees

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Ryan VanderMeulen [:RyanVM]]

We had a long tree closure tonight where a contributing factor was the fact that browser_aboutHome.js touches the outside network, received a CSS error, and proceeded to dump large amounts of spam into the logs, causing us to hit the 50MB limit.

As a workaround, browser_aboutHome.js was disabled on trunk (and other trees will be following suit soon as well due to also being burned by this). This bug tracks it being re-enabled on all trees once it is fixed to not touch the outside network during operation.

Comment 1

[Bill Gianopoulos [:WG9s]]

Please expain why or how abouthome was ever doing network access????

Comment 2

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

The FHR portions of this test should be split into a separate test.

In general, this test should be broken down much more than it is, to hopefully work around many of the various issues it has.

Comment 3

[Bill Gianopoulos [:WG9s]]

You should be able to use Firefox on a completely isolated no internet access network, so abouthome being dependent on internet access is really just wrong.

Comment 4

[Marco Bonardo [:mak]]

about:home doesn't depend on network, it's just the FHR tests added to it are simulating a search to google, likely the wrong way.

Comment 5

[Marco Bonardo [:mak]]

adding as dependencies some related failures that may be taken into account when working on this, splitting fhr tests should solve most of the issues, the original test didn't trigger so many issues (some crashes maybe, but I think those are good, looks like the page is able to activate parts of the codebase not covered by other tests, and if that ends up in a crash we can fix, that's a win).

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Landed Gavin's disabling patch from bug 992485 on all affected release branches.

https://hg.mozilla.org/releases/mozilla-aurora/rev/ff61d8a0f93e
https://hg.mozilla.org/releases/mozilla-beta/rev/b1e9827af66f
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/a38eaa2ca3c1
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/01f376ea42e6
https://hg.mozilla.org/releases/mozilla-esr24/rev/fc31b14a46ea

Comment 7

[Drew Willcoxon :adw]

Attachment: Use a progress listener to really stop the page load

This is what browser_{google,bing}_behavior.js do and they've been working fine.

This bug is really independent of splitting up the test, so I'd rather not scope creep and do more.  We should file additional bugs, add them to the backlog, and work on them as part of the iteration process.

Comment 8

[Marco Bonardo [:mak]]

(In reply to Drew Willcoxon :adw from comment #7)
> This bug is really independent of splitting up the test, so I'd rather not
> scope creep and do more.  We should file additional bugs, add them to the
> backlog, and work on them as part of the iteration process.

I think those bugs are already filed, at least bug 916405 for splitting up the test (and I think splitting on fhr part would be a good implementation idea for that bug), it's also in the backlog already.

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 8409266 [details] [diff] [review]
Use a progress listener to really stop the page load

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

fwiw, this is the kind of helper that would be useful in head.js, since looks like the right way to check an url is accessed but not loaded at all. Having it in a common helper would make trivial to fix other similar issues.
On the other side, we don't usually code thinking of how to fix future bugs, so we could move it there when needed.  Your call, the fix looks good.

::: browser/base/content/test/general/browser_aboutHome.js
@@ +137,5 @@
> +          let docStart = Ci.nsIWebProgressListener.STATE_IS_DOCUMENT |
> +                         Ci.nsIWebProgressListener.STATE_START;
> +          if ((flags & docStart) && webProgress.isTopLevel) {
> +            info("Document start: " +
> +                 req.QueryInterface(Ci.nsIChannel).URI.spec);

what about rather checking this is is actually the url we were expecting to start loading?

Comment 10

[Drew Willcoxon :adw]

https://hg.mozilla.org/integration/fx-team/rev/3c9f46eaf824

I was reluctant to factor out the helper into head.js because you can imagine different callers wanting different options, and without specific uses cases I don't want to over-generalize it.  But on the other hand people aren't likely to adapt it to their use cases if it's not in head.js to begin with, so I added waitForDocLoadAndStopIt() it to head.js.

(In reply to Marco Bonardo [:mak] from comment #9)
> what about rather checking this is is actually the url we were expecting to
> start loading?

Done.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3c9f46eaf824

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7cd73a59acae