JavaScript error trying to use undo close tab menuitem in a private window

RESOLVED FIXED in seamonkey2.20

Status

SeaMonkey
Tabbed Browser
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: neil@parkwaycc.co.uk, Assigned: neil@parkwaycc.co.uk)

Tracking

(Blocks: 1 bug)

unspecified
seamonkey2.20

SeaMonkey Tracking Flags

(seamonkey2.18 fixed, seamonkey2.19 fixed, seamonkey2.20 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

4 years ago
Error: NS_ERROR_ILLEGAL_VALUE: 'Illegal value' when calling method: [nsISessionStore::getClosedTabData]
Source File: chrome://navigator/content/tabbrowser.xml
Line: 1513
(Assignee)

Comment 1

4 years ago
Created attachment 734118 [details] [diff] [review]
Proposed patch
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #734118 - Flags: review?(iann_bugzilla)
Attachment #734118 - Flags: feedback?(philip.chee)
(Assignee)

Comment 2

4 years ago
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

4 years ago
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

4 years ago
(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?
(Assignee)

Comment 5

4 years ago
Created attachment 734140 [details] [diff] [review]
Alternative patch

So, after all that, I went for a slightly different alternative approach ;-)
Attachment #734140 - Flags: review?(iann_bugzilla)
Attachment #734140 - Flags: feedback?(philip.chee)

Comment 6

4 years ago
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;
Attachment #734140 - Flags: feedback?(philip.chee) → feedback+
(Assignee)

Comment 7

4 years ago
(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.)

Updated

4 years ago
Attachment #734118 - Flags: feedback?(philip.chee)

Updated

4 years ago
Attachment #734140 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Comment 8

4 years ago
Pushed comm-central changeset 53106593a26c.
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-seamonkey2.20: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.20

Updated

4 years ago
Attachment #734118 - Flags: review?(iann_bugzilla)
(Assignee)

Comment 9

4 years ago
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
Attachment #734140 - Flags: approval-comm-beta?
Attachment #734140 - Flags: approval-comm-aurora?

Updated

4 years ago
Attachment #734140 - Flags: approval-comm-beta?
Attachment #734140 - Flags: approval-comm-beta+
Attachment #734140 - Flags: approval-comm-aurora?
Attachment #734140 - Flags: approval-comm-aurora+
(Assignee)

Comment 10

4 years ago
https://hg.mozilla.org/releases/comm-aurora/rev/7c29daab1e85
https://hg.mozilla.org/releases/comm-beta/rev/9a85f4353a9c
status-seamonkey2.18: affected → fixed
status-seamonkey2.19: affected → fixed
You need to log in before you can comment on or make changes to this bug.