Closed Bug 1313358 Opened 8 years ago Closed 4 years ago

mochitest --repeat breaks ContentSearch

Categories

(Firefox :: Search, defect, P3)

Product:
Firefox
Component:
Search
Type:
defect
Points:
2

Tracking

()

Status:
RESOLVED FIXED
Milestone:
86 Branch
Iteration:
86.3 - Jan 11 - Jan 24
Tracking Flags:
Tracking Status
firefox86 --- fixed

People

(Reporter: mak, Assigned: standard8)

Details

(Whiteboard: [fxsearch])

Attachments

(1 file)

Bug 1313358 - Clean up preference observer in ContentSearch. r?mak!
(deleted), text/x-phabricator-request
Details
I don't know if this can happen in other cases or with other mochitest options, but it surely happens with --repeat. The mochitest-browser harness fires "shutdown-leaks-before-check" that ContentSearch listens to, and when that happens it invokes its .destroy() method. ContentSearch.init() instead runs at "final-ui-startup". Looks like --repeat doesn't really cause a full shutdown/restart, it just reuses the same window, so we destroy the controller, but we never go through final-ui-startup and running the test again fails cause the controller is dead. We have various intermittent timeouts in m-b tests using ContentSearch and I wonder if there's a relation, or if this is just a bug in the harness.
Flags: needinfo?(adw)
I'll try to take a look at this soon. Is this urgent, Marco?
Assignee: nobody → adw
Status: NEW → ASSIGNED
Flags: needinfo?(adw)
A simple fix might be for ContentSearch to call init() on itself each time it receives a message -- each time it needs to do anything. init would have to be made idempotent of course.
no it's not urgent, it's just that it disallows investigating intermittents that involve ContentSearch and may cause devs to follow red herrings that are unrelated to the intermittent.
Whiteboard: [fxsearch]

I just tried ./mach mochitest browser/modules/test/browser/browser_ContentSearch.js --verify and ./mach mochitest browser/modules/test/browser/browser_ContentSearch.js --repeat 5 and it worked fine.

Do you know if there was a different test this was an issue on?

Flags: needinfo?(mak)

It was on any test using ContentSearch.jsm observers that are added by its init() call, so tests notifying these things:
https://searchfox.org/mozilla-central/rev/cce8b90aece0f42e5025e45282de16066eeaa662/browser/modules/ContentSearch.jsm#117

ContentSearch is initialized by browserGlue
https://searchfox.org/mozilla-central/rev/cce8b90aece0f42e5025e45282de16066eeaa662/browser/components/BrowserGlue.jsm#578
"shutdown-leaks-before-check" is fired here
https://searchfox.org/mozilla-central/rev/cce8b90aece0f42e5025e45282de16066eeaa662/testing/mochitest/browser-test.js#890
ContentSearch is listening for that here
https://searchfox.org/mozilla-central/rev/cce8b90aece0f42e5025e45282de16066eeaa662/browser/modules/ContentSearch.jsm#220

The problem is apparently there yet, maybe something changed in the last 3 years so that either --repeat doesn't send anymore "shutdown-leaks-before-check" or ContentSearch.jsm doesn't listen properly for it, or browser_ContentSearch.js doesn't test the observers added in init().

Flags: needinfo?(mak)

The only thing I can see wrong here is that we're not removing the preference observer in cleanup. Given that browser_contentSearchUI.js is definitely invoking and using ContentSearch, I think we should clean that up and call this done.

ContentSearch is also now invoked as an actor, so I don't know if that might have changed how the interop works as well.

Assignee: adw → standard8
Severity: normal → S4
Iteration: --- → 86.3 - Jan 11 - Jan 24
Points: --- → 2
Pushed by mbanner@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5fad54c908a8 Clean up preference observer in ContentSearch. r=mak

https://hg.mozilla.org/mozilla-central/rev/5fad54c908a8

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox86: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 86 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: