Nathan Froyd [:froydnj] Reporter
	Description • 10 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.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 1 • 10 years ago

(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?

	Nathan Froyd [:froydnj] Reporter
	Comment 2 • 10 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?

	Drew Willcoxon :adw Assignee
	Comment 3 • 10 years ago

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

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 4 • 10 years ago

Marco: can you  make sure this one is added to the current iteration properly?

	Marco Mucci [:MarcoM]
	Comment 5 • 10 years ago

Added to iteration.

	Marco Bonardo [:mak]
	Comment 6 • 10 years ago

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).

	Drew Willcoxon :adw Assignee
	Comment 7 • 10 years ago

Landed with 10 checks instead of 100:
https://hg.mozilla.org/integration/fx-team/rev/a8b48008fbe0

	Carsten Book [:Tomcat]
	Comment 8 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/a8b48008fbe0

	Ryan VanderMeulen [:RyanVM]
	Comment 9 • 10 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/3765981445e1
https://hg.mozilla.org/releases/mozilla-beta/rev/2b437d292f56
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/a33d76b72af1
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/7034533091e8
https://hg.mozilla.org/releases/mozilla-esr24/rev/ae70b479d2ff

	Ryan VanderMeulen [:RyanVM]
	Comment 10 • 10 years ago

Wow, totally nailed reading comment 0 there. Backed out from anything less than mozilla-beta since bug 995041 never got uplifted past there.

	Ryan VanderMeulen [:RyanVM]
	Comment 11 • 10 years ago

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/2b437d292f56