Closed Bug 995041 Opened 10 years ago Closed 10 years ago

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

Categories

(Firefox :: General, defect)

26 Branch
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 31
Tracking Status
firefox29 --- disabled
firefox30 --- fixed
firefox31 --- fixed
firefox-esr24 --- disabled
b2g-v1.2 --- disabled
b2g-v1.3 --- disabled
b2g-v1.3T --- disabled
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: RyanVM, Assigned: adw)

References

()

Details

(Whiteboard: p=3 s=it-31c-30a-29b.3 [qa-])

Attachments

(1 file)

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.
Please expain why or how abouthome was ever doing network access????
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.
Flags: firefox-backlog+
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.
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.
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).
Depends on: 916405, 968597, 981473
Blocks: 617414
Assignee: nobody → adw
Status: NEW → ASSIGNED
Whiteboard: p=3 s=it-31c-30a-29b.3 [qa?]
Whiteboard: p=3 s=it-31c-30a-29b.3 [qa?] → p=3 s=it-31c-30a-29b.3 [qa-]
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.
Attachment #8409266 - Flags: review?(mak77)
(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 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?
Attachment #8409266 - Flags: review?(mak77) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/3c9f46eaf824
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Status: RESOLVED → VERIFIED
Flags: needinfo?(ryanvm)
You need to log in before you can comment on or make changes to this bug.