Closed Bug 1232548 Opened 4 years ago Closed 4 years ago

browser_urlbarSearchTelemetry.js,browser_urlbarSearchSuggestions.js,browser_urlbarSearchSuggestionsNotification.js,browser_urlbarSearchSingleWordNotification.js leak until shutdown on mozilla-aurora 45

Categories

(Firefox :: Address Bar, defect, P1, blocker)

defect

Tracking

()

RESOLVED FIXED
Firefox 46
Tracking Status
firefox45 --- verified
firefox46 --- fixed

People

(Reporter: philor, Assigned: adw)

References

Details

(Whiteboard: [fxsearch])

Attachments

(1 file)

This is keeping mozilla-aurora from opening after the merge.

None of them are permaorange, browser_urlbarSearchTelemetry.js coming the closest, but nearly always at least one and mostly several of them will do

https://treeherder.mozilla.org/logviewer.html#?job_id=1621764&repo=mozilla-aurora


715 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_urlbarSearchTelemetry.js | leaked until shutdown [nsGlobalWindow #3294 inner http://mochi.test:8888/browser/browser/base/content/test/general/searchSuggestionEngine.sjs?searchsuggestionfoo http://mochi.test:8888/browser/browser/base/content/test/general/searchSuggestionEngine.sjs?searchsuggestionfoo] -
716 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_urlbarSearchSuggestions.js | leaked until shutdown [nsGlobalWindow #3258 inner http://mochi.test:8888/browser/browser/base/content/test/general/searchSuggestionEngine.sjs?foofoo http://mochi.test:8888/browser/browser/base/content/test/general/searchSuggestionEngine.sjs?foofoo] -
717 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_urlbarSearchSuggestionsNotification.js | leaked until shutdown [nsGlobalWindow #3291 outer http://mochi.test:8888/browser/browser/base/content/test/general/searchSuggestionEngine.sjs?searchsuggestionfoo] -
 718 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_urlbarSearchSingleWordNotification.js | leaked until shutdown [nsGlobalWindow #3256 outer http://mochi.test:8888/browser/browser/base/content/test/general/searchSuggestionEngine.sjs?foofoo] -
Blocks: 1233066
per irc flo will bring this up in the search meeting
Assignee: nobody → adw
Rank: 5
Priority: -- → P1
Whiteboard: [fxsearch]
Attached patch patchSplinter Review
No idea why these intermittently leak only on Aurora.  Guessing, I'd say that either the leaks aren't meaningful because it just so happens that the pages aren't collected in time, or that these are possibly real leaks and point to some bug around loading application/json pages in Gecko.

Two of these failing tests click results in the awesomebar, which loads the leaked URIs.  I'm guessing the other two failing tests are hit by those pages sticking around for them, too.

This patch makes a couple of changes.  No idea whether they'll make a difference.

(1) Open the clicked results in new tabs and close those tabs before the test finishes.

(2) Change the mock search engine so that GET requests for text/html (i.e., the requests that happen when the test clicks awesomebar results) actually return a text/html page instead of application/json search suggestions.

I'd ask Marco for his opinion on the patch before landing, but he's on vacation, and my understanding is that you don't actually have to get r+ before landing a test-only change.
Flags: needinfo?(wkocher)
Status: NEW → ASSIGNED
Is the JSON viewer thing devtools shipped only enabled by default on aurora or something? That might explain why opening application/json pages does something different there than on nightly. If that is the case, we should probably still investigate if that viewer leaks in some way... Jan, can you clarify where the viewer is currently turned on by default?
Flags: needinfo?(odvarko)
Comment on attachment 8699250 [details] [diff] [review]
patch

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

::: browser/base/content/test/general/browser_urlbarSearchSuggestions.js
@@ +21,5 @@
>    });
>  });
>  
>  add_task(function* clickSuggestion() {
> +  gBrowser.selectedTab = gBrowser.addTab();

Nit: FWIW, on Nightly it might make sense to use BrowserTestUtils.withNewTab() or something like that in tests like this one.
https://hg.mozilla.org/mozilla-central/rev/31c4e5dbd4c3
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
(In reply to :Gijs Kruitbosch (away 18-28 Dec.) from comment #7)
> Is the JSON viewer thing devtools shipped only enabled by default on aurora
> or something? That might explain why opening application/json pages does
> something different there than on nightly. If that is the case, we should
> probably still investigate if that viewer leaks in some way... Jan, can you
> clarify where the viewer is currently turned on by default?

Not Jan, but it was my understanding (and experience) that it was only preffed on by default for aurora.
(In reply to Wes Kocher (:KWierso) (On vacation until Dec 28) from comment #10)
> (In reply to :Gijs Kruitbosch (away 18-28 Dec.) from comment #7)
> > Is the JSON viewer thing devtools shipped only enabled by default on aurora
> > or something? That might explain why opening application/json pages does
> > something different there than on nightly. If that is the case, we should
> > probably still investigate if that viewer leaks in some way... Jan, can you
> > clarify where the viewer is currently turned on by default?
> 
> Not Jan, but it was my understanding (and experience) that it was only
> preffed on by default for aurora.
That is correct, JSON Viewer is enabled by default only in Aurora (DevEdition)
(sorry for the delay, just got back from PTO)


Honza
Flags: needinfo?(odvarko)
(In reply to Jan Honza Odvarko [:Honza] from comment #12)
> (In reply to Wes Kocher (:KWierso) (On vacation until Dec 28) from comment
> #10)
> > (In reply to :Gijs Kruitbosch (away 18-28 Dec.) from comment #7)
> > > Is the JSON viewer thing devtools shipped only enabled by default on aurora
> > > or something? That might explain why opening application/json pages does
> > > something different there than on nightly. If that is the case, we should
> > > probably still investigate if that viewer leaks in some way... Jan, can you
> > > clarify where the viewer is currently turned on by default?
> > 
> > Not Jan, but it was my understanding (and experience) that it was only
> > preffed on by default for aurora.
> That is correct, JSON Viewer is enabled by default only in Aurora
> (DevEdition)
> (sorry for the delay, just got back from PTO)
> 
> 
> Honza

Right, in which case, can you investigate if/why the viewer leaks without this patch?
Flags: needinfo?(odvarko)
(In reply to :Gijs Kruitbosch from comment #13)
> Right, in which case, can you investigate if/why the viewer leaks without
> this patch?
I believe that leaks are related to 'promiseTabLoaded' API in the test. This function waits till 'load' event is fired from the content, but JSON Viewer isn't done yet and => the window leaks.

There is an event "JSONViewInitialized" that is fired from the content when JSON Viewer is fully initialized:
https://github.com/mozilla/gecko-dev/blob/master/devtools/client/jsonview/json-viewer.js#L94

Utilized in tests already:
https://github.com/mozilla/gecko-dev/blob/master/devtools/client/jsonview/test/doc_frame_script.js#L31

Note that JSON Viewer is currently using RequireJS to load all modules (this is async and takes the extra time after 'load' event). We are planning to use a package manager to produce a single bundle that would be loaded using <script> tag. I think this could solve the problem too. I reported bug 1237253 for this.

Honza
Flags: needinfo?(odvarko)
You need to log in before you can comment on or make changes to this bug.