Closed Bug 880361 Opened 6 years ago Closed 6 years ago

"browser is undefined" error when immediately closing a newly opened browser window

Categories

(Firefox :: General, defect)

x86_64
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 24

People

(Reporter: jruderman, Assigned: adw)

References

(Blocks 1 open bug)

Details

(Keywords: regression, testcase)

Attachments

(2 files)

Attached file testcase
[Exception... "'[JavaScript Error: "browser is undefined" {file: "chrome://browser/content/browser.js" line: 1763}]' when calling method: [nsIContentPrefCallback2::handleCompletion]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: resource://gre/modules/ContentPrefService2.jsm :: safeCallback :: line 797"  data: yes]

Likely a regression from one of these two merges into central:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=204de5b7e0a6&tochange=a47f4e36197f

In particular, I suspect http://hg.mozilla.org/mozilla-central/rev/b1da96acb0dc, which changed both files mentioned in the error message (browser-fullZoom.js and ContentPrefService2.jsm).
Sounds like the content pref callbacks need to be able to deal with the window being destroyed (or we need a way to cancel pending content pref queries on window close).
The JS stack:

FullZoom__getState@chrome://browser/content/browser.js:1765
FullZoom__isStateCurrent@chrome://browser/content/browser.js:1776
FullZoom_onLocationChange/<.handleCompletion<@chrome://browser/content/browser.js:1598
safeCallback@resource://gre/modules/ContentPrefService2.jsm:797
cbHandleCompletion@resource://gre/modules/ContentPrefService2.jsm:786
onDone@resource://gre/modules/ContentPrefService2.jsm:92
handleCompletion@resource://gre/modules/ContentPrefService2.jsm:663

FullZoom.onLocationChange is called for the page in the new window, which starts an async cps2.getByDomainAndName.  When that finishes, the current FullZoom state is captured, which accesses the selected browser, which is null because the window is already closed.  _getState shouldn't assume the browser is nonnull.

It's fine for _getState to be called in this case.  That's not the problem.  _isStateCurrent is the only place that checks the uri property of the object that _getState returns, and it already guards against null uris and does the right thing in that case.

This also fixes the JavaScript strict warning, "variable browser redeclares argument", whoops.
Assignee: nobody → adw
Status: NEW → ASSIGNED
Attachment #759419 - Flags: review?(gavin.sharp)
Comment on attachment 759419 [details] [diff] [review]
don't assume browser is nonnull

This could certainly use a comment explaining why you're null checking selectedBrowser (which is otherwise generally never null). I.e. "this function can run after the window is unloaded". Probably worth putting a comment like that both here and in _isStateCurrent.
Attachment #759419 - Flags: review?(gavin.sharp) → review+
https://hg.mozilla.org/mozilla-central/rev/bbe73080e0e8
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
You need to log in before you can comment on or make changes to this bug.