Last Comment Bug 858825 - JavaScript error trying to use undo close tab menuitem in a private window
: JavaScript error trying to use undo close tab menuitem in a private window
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Tabbed Browser (show other bugs)
: unspecified
: All All
: -- normal (vote)
: seamonkey2.20
Assigned To: neil@parkwaycc.co.uk
:
Mentors:
Depends on:
Blocks: 460895
  Show dependency treegraph
 
Reported: 2013-04-05 15:49 PDT by neil@parkwaycc.co.uk
Modified: 2013-04-21 02:38 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed
fixed
fixed


Attachments
Proposed patch (3.84 KB, patch)
2013-04-05 15:53 PDT, neil@parkwaycc.co.uk
no flags Details | Diff | Splinter Review
Alternative patch (6.53 KB, patch)
2013-04-05 16:46 PDT, neil@parkwaycc.co.uk
iann_bugzilla: review+
philip.chee: feedback+
iann_bugzilla: approval‑comm‑aurora+
iann_bugzilla: approval‑comm‑beta+
Details | Diff | Splinter Review

Description neil@parkwaycc.co.uk 2013-04-05 15:49:57 PDT
Error: NS_ERROR_ILLEGAL_VALUE: 'Illegal value' when calling method: [nsISessionStore::getClosedTabData]
Source File: chrome://navigator/content/tabbrowser.xml
Line: 1513
Comment 1 neil@parkwaycc.co.uk 2013-04-05 15:53:05 PDT
Created attachment 734118 [details] [diff] [review]
Proposed patch
Comment 2 neil@parkwaycc.co.uk 2013-04-05 15:57:37 PDT
Oh, I guess it would help to actually say something.

Session store doesn't activate for private windows, so it has no idea what is going on.

Therefore I have wrapped all the calls to session store in try/catch so if it fails we just use the old internal undo close tab mechanism.

I suppose the alternative would be for navigator.js to forward the privacy status to the tabbrowser so it could use if/else instead of try/catch.
Comment 3 Ian Neal 2013-04-05 15:59:06 PDT
Comment on attachment 734118 [details] [diff] [review]
Proposed patch

>+      <method name="undoCloseTabDisabled">
>+        <body>
>+          <![CDATA[
>+            try {
>+              this.mSessionStore.getClosedTabCount(window);
>+              if (this.mPrefs.getIntPref("browser.sessionstore.max_tabs_undo") > 0)
>+                return false;
>+            } catch (e) {
>+              // no session restore, just use saved browsers
Is this comment actually true in this case, surely it is a pref we are using here?
>+            }
>+            return this.mPrefs.getIntPref("browser.tabs.max_tabs_undo") <= 0;
>+          ]]>
>+        </body>
>+      </method>
Comment 4 Ian Neal 2013-04-05 16:03:21 PDT
(In reply to neil@parkwaycc.co.uk from comment #2)
> Oh, I guess it would help to actually say something.
> 
> Session store doesn't activate for private windows, so it has no idea what
> is going on.
> 
> Therefore I have wrapped all the calls to session store in try/catch so if
> it fails we just use the old internal undo close tab mechanism.
> 
> I suppose the alternative would be for navigator.js to forward the privacy
> status to the tabbrowser so it could use if/else instead of try/catch.

Is using navigator.js any more efficient?
Comment 5 neil@parkwaycc.co.uk 2013-04-05 16:46:23 PDT
Created attachment 734140 [details] [diff] [review]
Alternative patch

So, after all that, I went for a slightly different alternative approach ;-)
Comment 6 Philip Chee 2013-04-09 10:42:43 PDT
Comment on attachment 734140 [details] [diff] [review]
Alternative patch

I like this patch better. f=me

> +      <field name="usePrivateBrowsing" readonly="true">
> +        window.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
> +              .getInterface(Components.interfaces.nsIWebNavigation)
> +              .QueryInterface(Components.interfaces.nsILoadContext)
> +              .usePrivateBrowsing
Shouldn't there be a semi-colon after .usePrivateBrowsing ?

Can we use the following instead?:
this.mCurrentBrowser.docShell.QueryInterface(Components.interfaces.nsILoadContext).usePrivateBrowsing;
Comment 7 neil@parkwaycc.co.uk 2013-04-09 11:37:34 PDT
(In reply to Philip Chee from comment #6)
> (From update of attachment 734140 [details] [diff] [review])
> > +      <field name="usePrivateBrowsing" readonly="true">
> > +        window.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
> > +              .getInterface(Components.interfaces.nsIWebNavigation)
> > +              .QueryInterface(Components.interfaces.nsILoadContext)
> > +              .usePrivateBrowsing
> Shouldn't there be a semi-colon after .usePrivateBrowsing ?
We're using it as an expression, rather than a statement.

> Can we use the following instead?:
> this.mCurrentBrowser.docShell.QueryInterface(Components.interfaces.
> nsILoadContext).usePrivateBrowsing;
I don't want us to be confused by that private tab extension. (In theory we could check the window or the tab as appropriate for the call, but session store wants a window-level check anyway.)
Comment 8 neil@parkwaycc.co.uk 2013-04-13 16:19:36 PDT
Pushed comm-central changeset 53106593a26c.
Comment 9 neil@parkwaycc.co.uk 2013-04-18 13:32:39 PDT
Comment on attachment 734140 [details] [diff] [review]
Alternative patch

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: Undo close menuitems don't work in private windows
Testing completed (on m-c, etc.): Baked on c-c
Risk to taking this patch (and alternatives if risky): Maybe, but not a lot
String changes made by this patch: None

Note You need to log in before you can comment on or make changes to this bug.