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

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Jesse Ruderman]

Attachment: 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).

Comment 1

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

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).

Comment 2

[Drew Willcoxon :adw]

Attachment: don't assume browser is nonnull

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.

Comment 3

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

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.

Comment 4

[Drew Willcoxon :adw]

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

Comment 5

[Ryan VanderMeulen [:RyanVM]]

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