993013 - this._cps2 is undefined (at browser.js:1705)

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

I get this JS error in my Browser Console when a window is restored (through Session Restore).

The code in question:
    // See if the zoom pref is cached.
    let ctxt = this._loadContextFromBrowser(browser);
    let pref = this._cps2.getCachedByDomainAndName(aURI.spec, this.name, ctxt);
    if (pref) {
      this._applyPrefToZoom(pref.value, browser,
                            this._notifyOnLocationChange.bind(this));
      return;
    }

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0003-Bug-993013-Fix-this._cps2-is-undefined-errors-in-tes.patch

Comment 2

[Marco Mucci [:MarcoM]]

Hi Tim, can you provide a point value and if the bug should be marked as [qa+] or [qa-]

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Sure :)

Comment 4

[Drew Willcoxon :adw]

Comment on attachment 8442111 [details] [diff] [review]
0003-Bug-993013-Fix-this._cps2-is-undefined-errors-in-tes.patch

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

::: browser/base/content/browser-fullZoom.js
@@ +219,5 @@
>     *        (optional) browser object displaying the document
>     */
>    onLocationChange: function FullZoom_onLocationChange(aURI, aIsTabSwitch, aBrowser) {
> +    // TabsProgressListener might call us before the delayed startup had a
> +    // a chance to call FullZoom.init(). Bail if we're not initialized, yet.

Hmm, if that's the case, and the page is in the selected tab, does it mean that the page will just have the wrong zoom (until you tab away from it and then back to it)?  Should we queue the location change and handle it after init?

Übernit: the comma before the "yet" isn't necessary. :-)

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-993013-Fix-this._cps2-is-undefined-errors-in-tes.patch, v2

I think you're right. There are multiple onLocationChange() notifications for the first tab when restoring a window but to be on the safe side we could just record the "initial location" for browsers and replay those when being initialized.

Comment 6

[Drew Willcoxon :adw]

Comment on attachment 8445854 [details] [diff] [review]
0001-Bug-993013-Fix-this._cps2-is-undefined-errors-in-tes.patch, v2

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

Cool, thanks.

::: browser/base/content/browser-fullZoom.js
@@ +67,5 @@
> +
> +    // If we received onLocationChange events for any of the current browsers
> +    // before we were initialized we want to replay those upon initialization.
> +    for (let browser of gBrowser.browsers) {
> +      if (this._initialLocations.has(browser)) {

Is there any way you could iterate over the map instead of all open tabs, checking if each browser is still open?  There could be tons of tabs open, but the map should always be pretty small.

@@ +69,5 @@
> +    // before we were initialized we want to replay those upon initialization.
> +    for (let browser of gBrowser.browsers) {
> +      if (this._initialLocations.has(browser)) {
> +        let params = this._initialLocations.get(browser);
> +        this.onLocationChange.apply(this, [...params, browser]);

this.onLocationChange(...params, browser);

:-D

Comment 7

[Drew Willcoxon :adw]

(In reply to Drew Willcoxon :adw from comment #6)
> Is there any way you could iterate over the map instead of all open tabs,
> checking if each browser is still open?  There could be tons of tabs open,
> but the map should always be pretty small.

Or is FullZoom.onLocationChange called for each open tab before init?  Nevermind if so.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Drew Willcoxon :adw from comment #6)
> > +        this.onLocationChange.apply(this, [...params, browser]);
> 
> this.onLocationChange(...params, browser);
> 
> :-D

Hah, good call.

(In reply to Drew Willcoxon :adw from comment #7)
> (In reply to Drew Willcoxon :adw from comment #6)
> > Is there any way you could iterate over the map instead of all open tabs,
> > checking if each browser is still open?  There could be tons of tabs open,
> > but the map should always be pretty small.
> 
> Or is FullZoom.onLocationChange called for each open tab before init? 
> Nevermind if so.

You're right that the map will always be small, mostly zero. I hope that iterating over all browsers shouldn't be a big perf hit as iterating the keys of a WeakMap is not possible. There is Cu.nondeterministicGetWeakMapKeys() but that should only be used for tests according to its description.

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/4884b0efae7f

Comment 10

[Drew Willcoxon :adw]

(In reply to Tim Taubert [:ttaubert] (away July 7th-18th) from comment #8)
> iterating the keys of a WeakMap is not possible.

Oh, too bad. :-(

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4884b0efae7f