867833 - some code expects urlbar to always exist

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect)

Description

[Shane Caraveo (:mixedpuppy)]

not sure if this is the best component for this bug.

browser/base/content/test/browser_customize_popupNotification.js is removing the urlbar, which shows that at least some code expects the urlbar to exist.  The tests pass but you see the exceptions.

 0:04.09 *** Start BrowserChrome Test Results ***
 0:04.13 TEST-INFO | checking window state
 0:04.22 TEST-START | chrome://mochitests/content/browser/browser/base/content/test/browser_customize_popupNotification.js
 0:04.31 TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/browser_customize_popupNotification.js | Console message: OpenGL LayerManager Initialized Succesfully.
 0:04.31 Version: 2.1 APPLE-7.32.12
 0:04.31 Vendor: Intel Inc.
 0:04.31 Renderer: Intel HD Graphics 3000 OpenGL Engine
 0:04.31 FBO Texture Target: TEXTURE_2D
 0:04.32 TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/browser_customize_popupNotification.js | must wait for focus
 0:04.53 TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/browser_customize_popupNotification.js | showed the notification
 0:04.53 TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/browser_customize_popupNotification.js | panel is open
 0:04.53 TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/browser_customize_popupNotification.js | notification is correctly anchored to the tab
 0:04.53 TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/browser_customize_popupNotification.js | Console message: [JavaScript Error: "TypeError: gURLBar.select is not a function" {file: "chrome://browser/content/browser.js" line: 9951}]
 0:04.58 INFO TEST-END | chrome://mochitests/content/browser/browser/base/content/test/browser_customize_popupNotification.js | finished in 351ms
 0:04.58 TEST-INFO | checking window state
 0:04.64 TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/browser_customize_popupNotification.js | Console message: [JavaScript Error: "TypeError: this._cps2 is undefined" {file: "chrome://browser/content/browser.js" line: 1390}]
 0:04.86
 0:04.86 INFO TEST-START | Shutdown
 0:04.87 Browser Chrome Test Summary
 0:04.87 	Passed: 3
 0:04.87 	Failed: 0
 0:04.87 	Todo: 0
 0:04.87
 0:04.87 *** End BrowserChrome Test Results ***

Comment 1

[Shane Caraveo (:mixedpuppy)]

Attachment: fix cruft exceptions caused by test

The problem here is that the test was removing the urlbar prior to _delayedStartup (in browser.js) running.  During the waitForFocus in the test, delayedStartup ran but an exception occurs because the urlbar is gone, and much initialization is skipped.  Then when the window is closed, more exceptions occur in browser.js onUnload, resulting in some items that did get initalized not properly shutting down.  Even though not visible, the window sticks around during the test run (I don't know why), and various exceptions occur during the consecutive tests.

The changes make the test window fully appear prior to removing the urlbar and running the tests.  That allows the window to fully initialize, which allows unload to work as well.  

I had to additionally open a tab to make sure the urlbar was not removed prior to delayed startup running.  Even listening for the delayed startup notification wasn't enough to get the full initialization completed (and avoid exceptions in onUnload).  If there is a better notification to listen for let me know.

No more exceptions in log output, I'm happy now.

https://tbpl.mozilla.org/?tree=Try&rev=f9a97e118372

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Can we just re-use the whenDelayedStartupFinished helper from some of the other tests?

It doesn't seem necessary to actually wait for the load of a new tab in the new window.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #1)
> Even listening for the delayed startup notification wasn't enough to get the full
> initialization completed (and avoid exceptions in onUnload).  If there is a better
> notification to listen for let me know.

Oh, missed this. What exceptions in onUnload occurred when you did this?

Comment 4

[Shane Caraveo (:mixedpuppy)]

Attachment: fix cruft exceptions caused by test

Yes this works.  My previous attempt to use the "browser-delayed-startup-finished" notification wasn't quite right, I wasn't verifying the window as whenDelayedStartupFinished is doing.  This patch works as well as the last one and doesn't have to do the new tab.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 744913 [details] [diff] [review]
fix cruft exceptions caused by test

awesome, thanks!

Comment 6

[Shane Caraveo (:mixedpuppy)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/898891617618

Comment 7

[Ed Morley [:emorley]]

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