Closed Bug 1188692 Opened 9 years ago Closed 7 years ago

Intermittent browser_ContentSearch.js | application terminated with exit code 1 | after Non-local network connections are disabled and a connection attempt to www.google.com (64.233.177.105) was made.

Categories

(Firefox :: Search, defect, P3)

Product:
Firefox
Component:
Search
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 53
Tracking Flags:
Tracking Status
firefox48 --- wontfix
firefox49 --- wontfix
firefox50 --- wontfix
firefox51 --- fixed
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: KWierso, Assigned: standard8)

References

Details

(Keywords: intermittent-failure)

Attachments

(1 file)

Bug 1188692 - Attempt to reduce intermittent failures in browser_ContentSearch.js by initialising listeners earlier.
59 bytes, text/x-review-board-request
adw
: review+
Details 
This should be fixed by bug 1249332 when it ultimately lands.
Depends on: 1249332
Bulk assigning P3 to all open intermittent bugs without a priority set in Firefox components per bug 1298978.
Priority: -- → P3
(In reply to Drew Willcoxon :adw from comment #10)
> This should be fixed by bug 1249332 when it ultimately lands.

Looks like that didn't work. Any other thoughts?
status-firefox48: --- → wontfix
status-firefox49: --- → wontfix
status-firefox50: --- → affected
status-firefox51: --- → affected
Flags: needinfo?(adw)
I'll try to look at this but I probably won't be able to very soon.
Assignee: nobody → adw
Status: NEW → ASSIGNED
Flags: needinfo?(adw)
:adw -- This has been failing for a long time now. Could it be disabled? Alternately, do you have a strategy in mind for fixing it?
Flags: needinfo?(adw)
I don't think it should be disabled since that would mean the search fields in about:home and about:newtab would lose a lot of their test coverage.  Looking at the comment timestamps in this bug, it's fairly intermittent.  And the failure reason is the no-network-connections thing, which makes me think this is not a meaningful failure, but I could be wrong.  I'll try to prioritize this more highly, but to be honest it's the end of the quarter and I have more important things to finish.
Flags: needinfo?(adw)
The no-network-connections thing happens when a test tries to access an external site; we often avoid that with test preferences like https://hg.mozilla.org/mozilla-central/annotate/47f42f21541b9b98ad7db82edb996b29065debd0/testing/profiles/prefs_general.js#l98. It looks like the failures are trying to access "https://www.google.com/search?q=ContentSearchTest&ie=utf-8&oe=utf-8".
I cannot seem to get this to fail for me locally on windows 10.

this test case doesn't run with --repeat 1, it seems to get stuck.

I also see this in the error console locally (win 10):
Unknown source for search: ContentSearchTest

there is a lot of code in this test, it will take some time to sift through all of this unless there is someone who knows the code better.

This is primarily on Windows 8- but seen on all systems, so this looks like some timing issue with the test and trying to use a different search engine than we think it is.
(In reply to Joel Maher ( :jmaher) from comment #51)
> I cannot seem to get this to fail for me locally on windows 10.
> 
> this test case doesn't run with --repeat 1, it seems to get stuck.

I took a look at this specific issue, and it appears that ContentSearch.jsm gets "destroyed" at "shutdown-leaks-before-check", this was added by bug 1057386.

The "shutdown-leaks-before-check" event gets sent just before the repeat happens, hence the failure of the test the second time through. To get around this, we'd probably need to trigger "final-ui-startup" somehow, or call ContentSearch.init() again...
(In reply to Joel Maher ( :jmaher) from comment #51)
> I also see this in the error console locally (win 10):
> Unknown source for search: ContentSearchTest

That's basically just a warning and isn't consequential for this test. Its a result of trying to log a telemetry result.

I took a dig through and the only thing that was obvious that I could see was when we're trying to stop the load from happening (in contentSearch.js), we set up that listener after forwarding the test event to the content service.

Although I'd expect the async nature of things to allow that listener to be set up before the test event gets fully processed, it feels a possible weak point.

I've therefore set up a try build to see if it helps:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab32b2a93b06dab3f1e213bdf65eb6381e7ab038
unfortunately we are still seeing frequent failures, although I think at about 1/3 the rate of before the patch.  The patch does help though :)
Comment on attachment 8821246 [details]
Bug 1188692 - Attempt to reduce intermittent failures in browser_ContentSearch.js by initialising listeners earlier.

https://reviewboard.mozilla.org/r/100556/#review103180

Thanks Mark, sorry for the delay.  This looks fine, so r+.  It is surprising to me that this reduces the failures since the search only starts after content sends an async message to ContentSearch.jsm.  So dispatching the event after adding the waitForLoadAndStopIt listener shouldn't be any different from doing it before, I think.  But if it works, it works. :-)

I think there are a few ways of really fixing this, if you're interested.  Can be done in a new bug of course, if you like.

(1) Fix waitForLoadAndStopIt since it's obviously not working right.  I think there are a couple of implementations of that function in the tree, or at least there used to be.  We had problems with one or more of them in the past, same symptom.  Maybe the other one works right.  Or maybe it didn't work right either and we replaced it with a different approach altogether.  I don't remember.  So this might be a bad option.

(2) Monkey-patch ContentSearch.performSearch() in the test so that it doesn't actually search.  https://dxr.mozilla.org/mozilla-central/source/browser/modules/ContentSearch.jsm#223

(3) Install a dummy search engine for the test, like we do in plenty of other tests.  This is probably the best option.
Attachment #8821246 - Flags: review?(adw) → review+
Assignee: adw → standard8
Comment on attachment 8821246 [details]
Bug 1188692 - Attempt to reduce intermittent failures in browser_ContentSearch.js by initialising listeners earlier.

Thank you for the comments, Drew.

I went with the option of installing the dummy search engine. I've already been running this through try, and so far it seems to have fixed it:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7af9f72a3615e54be441fd6752cbf56ee51637ed
Attachment #8821246 - Flags: review+ → review?(adw)
Comment on attachment 8821246 [details]
Bug 1188692 - Attempt to reduce intermittent failures in browser_ContentSearch.js by initialising listeners earlier.

https://reviewboard.mozilla.org/r/100556/#review104240

Great, thanks.
Attachment #8821246 - Flags: review?(adw) → review+
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1ce407daf243
Attempt to reduce intermittent failures in browser_ContentSearch.js by initialising listeners earlier. r=adw
https://hg.mozilla.org/mozilla-central/rev/1ce407daf243
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: