Closed Bug 993013 Opened 9 years ago Closed 8 years ago

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

Categories

(Firefox :: General, defect)

defect
Not set
normal
Points:
1

Tracking

()

RESOLVED FIXED
Firefox 33
Iteration:
33.2

People

(Reporter: jaws, Assigned: ttaubert)

References

Details

Attachments

(1 file, 1 obsolete file)

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;
    }
Flags: firefox-backlog+
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Attachment #8442111 - Flags: review?(adw)
Hi Tim, can you provide a point value and if the bug should be marked as [qa+] or [qa-]
Flags: needinfo?(ttaubert)
Whiteboard: p=0 s=33.1 [qa?]
Sure :)
Flags: needinfo?(ttaubert)
Whiteboard: p=0 s=33.1 [qa?] → p=1 s=33.1 [qa-]
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. :-)
Attachment #8442111 - Flags: review?(adw) → review+
Iteration: --- → 33.2
Points: --- → 1
QA Whiteboard: [qa-]
Whiteboard: p=1 s=33.1 [qa-]
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.
Attachment #8442111 - Attachment is obsolete: true
Attachment #8445854 - Flags: review?(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
Attachment #8445854 - Flags: review?(adw) → review+
(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.
(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.
(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. :-(
https://hg.mozilla.org/mozilla-central/rev/4884b0efae7f
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
You need to log in before you can comment on or make changes to this bug.