1313358 - mochitest --repeat breaks ContentSearch

Status: RESOLVED FIXED

(Firefox :: Search, defect, P3)

Description

[Marco Bonardo [:mak]]

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.

Comment 1

[Drew Willcoxon :adw]

I'll try to take a look at this soon.  Is this urgent, Marco?

Comment 2

[Drew Willcoxon :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.

Comment 3

[Marco Bonardo [:mak]]

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.

Comment 4

[Mark Banner (:standard8)]

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?

Comment 5

[Marco Bonardo [: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().

Comment 6

[Mark Banner (:standard8)]

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.

Comment 7

[Mark Banner (:standard8)]

Attachment: Bug 1313358 - Clean up preference observer in ContentSearch. r?mak!

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5fad54c908a8
Clean up preference observer in ContentSearch. r=mak

Comment 9

[Narcis Beleuzu [:NarcisB]]

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