Closed Bug 1148787 Opened 5 years ago Closed 5 years ago

Fix a JS error when opening a new tab

Categories

(Firefox for Android :: General, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 39
Tracking Status
firefox39 --- fixed

People

(Reporter: ehsan, Assigned: ehsan)

Details

Attachments

(1 file)

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 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
Attachment #8585057 - Flags: review?(bnicholson) → review+
(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?
Flags: needinfo?(mark.finkle)
(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
Flags: needinfo?(mark.finkle)
https://hg.mozilla.org/mozilla-central/rev/2ac3d0135bba
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
You need to log in before you can comment on or make changes to this bug.