browser/base/content/test/general/browser_urlbar_search_healthreport.js attempts to connect to www.google.com

VERIFIED FIXED in Firefox 30

Status

()

defect
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: froydnj, Assigned: adw)

Tracking

unspecified
Firefox 32
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox30 fixed, firefox31 fixed, firefox32 fixed, firefox-esr24 wontfix, b2g-v1.2 wontfix, b2g-v1.3 wontfix, b2g-v1.3T wontfix, b2g-v1.4 fixed, b2g-v2.0 fixed)

Details

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

Attachments

(1 attachment)

Reporter

Description

5 years ago
With the patches in bug 995417 applied:

16:02:12     INFO -  TEST-START | chrome://mochitests/content/browser/browser/base/content/test/general/browser_urlbar_search_healthreport.js
16:02:12     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/general/browser_urlbar_search_healthreport.js | Health Reporter available.
16:02:12     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/general/browser_urlbar_search_healthreport.js | Searches provider is available.
16:02:12     INFO -  BAD CONNECT: connecting to www.google.com 0
16:02:13     INFO -  [2532] ###!!! ABORT: Aborting on channel error.: file /builds/slave/try-l64-0000000000000000000000/build/ipc/glue/MessageChannel.cpp, line 1522
16:02:13     INFO -  [2532] ###!!! ABORT: Aborting on channel error.: file /builds/slave/try-l64-0000000000000000000000/build/ipc/glue/MessageChannel.cpp, line 1522
16:02:13     INFO -  TEST-INFO | Main app process: killed by SIGSEGV

Looks like we're actually trying to do a search through the searchbox to verify that FHR records stats appropriately.  We need to search with a provider that we know connects to localhost, not google.com.
(In reply to Nathan Froyd (:froydnj) from comment #0)
> Looks like we're actually trying to do a search through the searchbox to
> verify that FHR records stats appropriately.  We need to search with a
> provider that we know connects to localhost, not google.com.

FHR only records searches for "known" providers. I suppose we can add a bogus "known" provider for testing purposes, but it risks somehow losing coverage of the case we actually care about (recording Google searches).

Bug 995041 is taking a different approach for a similar test that stops the request before it starts. Perhaps that would work here too?
Reporter

Comment 2

5 years ago
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #1)
> Bug 995041 is taking a different approach for a similar test that stops the
> request before it starts. Perhaps that would work here too?

That looks reasonable.  Drew, since you did the fix for bug 995041, can you do the fix for this bug, too?
Flags: needinfo?(adw)
Assignee

Comment 3

5 years ago
Posted patch patchSplinter Review
This stops the search page load when it starts and also polls for the new FHR measurement instead of doing two executeSoons.  The new measurement isn't yet available when waitForDocLoadAndStopIt is resolved.

https://tbpl.mozilla.org/?tree=Try&rev=6efaf2298f2f
Assignee: nobody → adw
Status: NEW → ASSIGNED
Attachment #8422025 - Flags: review?(mak77)
Flags: needinfo?(adw)
Marco: can you  make sure this one is added to the current iteration properly?
Flags: needinfo?(mmucci)
Flags: firefox-backlog+
Whiteboard: p=2 s=it-32c-31a-30b.2 [qa-]
Added to iteration.
Flags: needinfo?(mmucci)
Comment on attachment 8422025 [details] [diff] [review]
patch

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

::: browser/base/content/test/general/browser_urlbar_search_healthreport.js
@@ +52,5 @@
> +      // Meanwhile, poll for the new measurement.
> +      let count = 0;
> +      let measurementDeferred = Promise.defer();
> +      function getNewMeasurement() {
> +        if (count++ >= 100) {

nit: sounds very high when until today we waited just for a couple executeSoon... I'm under the impression 10 would be more than enough. IF more than those are needed, then the polling should be changed to a time-delay polling, or a custom event should be introduced.

fwiw, one of those is needed cause the fhr promise was resolved on the next tick while sdk promises were on the same tick. now this issue shouldn't exist anymore (indeed I guess laterTickResolvingPromise should be removed, I sent an email to Paolo about this).
Attachment #8422025 - Flags: review?(mak77) → review+
Assignee

Comment 7

5 years ago
Landed with 10 checks instead of 100:
https://hg.mozilla.org/integration/fx-team/rev/a8b48008fbe0
https://hg.mozilla.org/mozilla-central/rev/a8b48008fbe0
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
Status: RESOLVED → VERIFIED
Wow, totally nailed reading comment 0 there. Backed out from anything less than mozilla-beta since bug 995041 never got uplifted past there.
You need to log in before you can comment on or make changes to this bug.