1148787 - Fix a JS error when opening a new tab

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[(no longer active)]

I see this in logcat from a test run when I open a new window through window.open:

03-28 09:49:18.935 W/GeckoConsole(  592): [JavaScript Error: "TypeError: aBrowser.__SS_extdata is undefined" {file: "jar:jar:file:///data/app/org.mozilla.fennec-1.apk!/assets/omni.ja!/components/SessionStore.js" line: 351}]

I have a simple fix.

Comment 1

[(no longer active)]

Attachment: Ensure that the sessionstore properties exist on the browser object before reading them

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8585057 [details] [diff] [review]
Ensure that the sessionstore properties exist on the browser object before reading them


>   onTabClose: function ss_onTabClose(aWindow, aBrowser, aTabIndex) {
>     if (this._maxTabsUndo == 0)
>       return;
> 
>     if (aWindow.BrowserApp.tabs.length > 0) {
>       // Bundle this browser's data and extra data and save in the closedTabs
>       // window property
>-      let data = aBrowser.__SS_data;
>-      data.extData = aBrowser.__SS_extdata;
>+      let data = aBrowser.__SS_data || {};
>+      data.extData = aBrowser.__SS_extdata || {};

This code is about to push a closed Tab's data into the undo stack. With that in mind, I don't think we want to push an empty "data" object since that should never really be empty. It appears to be safe to push an empty "extdata" object since that object can be empty.

Can you just revert the | let data ... | line and keep the | data.extdata = ... | change?

It looks like that is enough to fix the failure you found.

I'll file a followup to do better checking on whether we should attempt to save tabs the are transient. It's something I should have filed a while ago.

r+ with change

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Filed bug 1148797

Comment 4

[(no longer active)]

(In reply to Mark Finkle (:mfinkle) from comment #2)
> Comment on attachment 8585057 [details] [diff] [review]
> Ensure that the sessionstore properties exist on the browser object before
> reading them
> 
> 
> >   onTabClose: function ss_onTabClose(aWindow, aBrowser, aTabIndex) {
> >     if (this._maxTabsUndo == 0)
> >       return;
> > 
> >     if (aWindow.BrowserApp.tabs.length > 0) {
> >       // Bundle this browser's data and extra data and save in the closedTabs
> >       // window property
> >-      let data = aBrowser.__SS_data;
> >-      data.extData = aBrowser.__SS_extdata;
> >+      let data = aBrowser.__SS_data || {};
> >+      data.extData = aBrowser.__SS_extdata || {};
> 
> This code is about to push a closed Tab's data into the undo stack. With
> that in mind, I don't think we want to push an empty "data" object since
> that should never really be empty. It appears to be safe to push an empty
> "extdata" object since that object can be empty.
> 
> Can you just revert the | let data ... | line and keep the | data.extdata =
> ... | change?

That is what I originally tried, but then I got another error on the same line because data was undefined.  Note that this code subtly accesses something off of the data variable on the left side of the assignment, which is why I need both of these lines changed.

Can I land as is?

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #4)

> That is what I originally tried, but then I got another error on the same
> line because data was undefined.  Note that this code subtly accesses
> something off of the data variable on the left side of the assignment, which
> is why I need both of these lines changed.
> 
> Can I land as is?

Sure. I can look into it more in bug 1148797

Comment 6

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2ac3d0135bba

Comment 7

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/2ac3d0135bba