1160407 - Pocket's "Open Pocket" and "Sign In"/"Create Account" link won't work well with Private Browsing

Status: RESOLVED FIXED

(Firefox :: Pocket, defect, P3)

Description

[Justin Dolske [:Dolske]]

The Pocket integration lives in chrome, so if the user was already signed in to Pocket, they will stay signed in and be able to save pages while in Private Browsing mode. This seems fine and expected.

However, the "Open Pocket" link in the Pocket page-saved panel basically just opens a link to getpocket.com in the current window. And that's not going to work in Private Browsing Mode very well, since unless the user has explicitly signed in to Pocket's web site in PB Mode they'll just get a login page. (I suspect the same will be true of the entry from the bookmarks menu?)

Should these point of the UI open the Pocket site in a new, non-private window in this case? And what happens in permanent private-browsing mode?

[And as a mildly interesting edge case: The user could be logged in to Pocket as UserA, open a private window, and log in to Pocket as UserB. The Pocket button will save to UserA's pocket, but the Open Pocket link will (currently) open Pocket as UserB in the private window.]

Comment 2

[(no longer active)]

Right now on Nightly, the Pocket toolbar button is disabled in private windows...

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #2)
> Right now on Nightly, the Pocket toolbar button is disabled in private
> windows...

It's only disable for chrome pages, it is enabled for non-chrome pages.

Comment 4

[Justin Dolske [:Dolske]]

Talked about this issue this morning: we'd like to make the "View Pocket List" menu items (in chrome) and the "View List" link (in the Pocket panel) either add a tab to your MRU non-Private window, or open up a new non-Private window.

Although now that I think about it more, there's still the case of permanent Private Browsing mode... I don't think there's much we can do there, since we can't open a non-Private window. (But I think that also means that you wouldn't be logged into Pocket anyway, unless you had logged in during your current session. So maybe that already Just Works.)

Comment 5

[(no longer active)]

Yeah, I would expect things to work already if the user has logged in to pocket in permanent private browsing.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Similar issues will happen with clicking on the Sign In button from the panel in a temporary Private Browsing window. If the user signs in on a temporary Private Browsing window, then the sign in will succeed but the panel will still say that the user is logged out.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

This patch fixes the case for temporary private browsing.

Permanent private browsing windows still don't work because pktApi.js uses nsICookieManager2.getCookiesFromHost which uses the non-private browsing cookie jar.

It looks like to get this working in private browsing mode, we will need to use nsICookieManager2.getCookiesForApp and pass in the Firefox app ID, preferably with a Pocket-related browser. Without the specific browser that will return *all* cookies for the app in the cookie jar.

Comment 8

[Nate Weiner]

Comment on attachment 8605511 [details] [diff] [review]
Patch

Review of attachment 8605511 [details] [diff] [review]:
-----------------------------------------------------------------

FYI: My review does not apply to the actual browser/cookie methods since I'm not as familiar with the internals of Firefox.

The only potential issue I see is that current expectation of openTabWithUrl is to open a new tab in the current window. As we discussed, for this particular issue, we want to handle opening the user's list in a more specific way (hence this code). However, I want to note that there are potentially more links that use that method and I'm not sure if we want to apply that behavior to _all_ links. 

In looking at it though, the only other link that we currently use this method for is the "Learn More" link in the Logged Out panel. While I think it's okay if in this rare case that it opened in a new window, it'd be a little odd. But to be honest, I'm not sure why that link is using that method vs just linking with target="_blank" like the others so we can make that adjustment to the panel code.

Long walk to say: This is fine for now, but we may want to allow the ability to open a tab in the current window (normal behavior) for other links as well.

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Alternative patch that disabled Pocket in temporary Private Browsing windows

This patch disables the Pocket integration points when in a temporary private browsing window. It seems less risk to me to take this.

One case that the other patch doesn't account for is if the user opened a PB window specifically for their Pocket usage to not be tracked. In that case, when they click on the "Log In" link it would open a tab in a non-private browser and leak the visit.

Comment 10

[Justin Dolske [:Dolske]]

Comment on attachment 8605511 [details] [diff] [review]
Patch

Review of attachment 8605511 [details] [diff] [review]:
-----------------------------------------------------------------

I am a little wary of risk that we've missed some edge case, or that users will expect Pocket to just be disabled in PB mode.

But this seems reasonable. And for the latter, I keep coming back to the fact (that we've managed to talk about but not note in this bug) that bookmarking already works in PB mode, and Pocket is basically just a better way to bookmark. So the two should work consistently.

::: browser/components/pocket/main.js
@@ +621,5 @@
>  
> +	function openTabWithUrl(url) {
> +        let recentWindow = Services.wm.getMostRecentWindow("navigator:browser");
> +        if (!recentWindow) {
> +#ifdef XP_MACOSX

See bug 1145394 -- should use AppConstants instead of preprocessing now.

Comment 11

[Justin Dolske [:Dolske]]

Comment on attachment 8606522 [details] [diff] [review]
Alternative patch that disabled Pocket in temporary Private Browsing windows

Review of attachment 8606522 [details] [diff] [review]:
-----------------------------------------------------------------

Let's go with the other fix, but since I was here...

::: browser/base/content/nsContextMenu.js
@@ +230,5 @@
>                                 (this.onSaveableLink || this.onPlainTextLink);
> +    this.showItem(saveLinkToPocketItem, showSaveLinkToPocket);
> +    if (showSaveLinkToPocket) {
> +      saveLinkToPocketItem.disabled = Pocket.shouldDisable(saveLinkToPocketItem);
> +    }

Oh, also need to disable/hide the Reader View's add-to-pocket button?

::: browser/components/pocket/Pocket.jsm
@@ +68,5 @@
>      window.pktUI.pocketPanelDidHide(event);
>    },
>  
> +  onCreated(node) {
> +    node.setAttribute("disabled", Pocket.shouldDisable(node));

Won't onLocationChange end up re-enabling the button? Seems like it would need a shouldDisable check too.

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Justin Dolske [:Dolske] from comment #11)
> Comment on attachment 8606522 [details] [diff] [review]
> Alternative patch that disabled Pocket in temporary Private Browsing windows
> 
> Review of attachment 8606522 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Let's go with the other fix, but since I was here...
> 
> ::: browser/base/content/nsContextMenu.js
> @@ +230,5 @@
> >                                 (this.onSaveableLink || this.onPlainTextLink);
> > +    this.showItem(saveLinkToPocketItem, showSaveLinkToPocket);
> > +    if (showSaveLinkToPocket) {
> > +      saveLinkToPocketItem.disabled = Pocket.shouldDisable(saveLinkToPocketItem);
> > +    }
> 
> Oh, also need to disable/hide the Reader View's add-to-pocket button?

Good catch.

> ::: browser/components/pocket/Pocket.jsm
> @@ +68,5 @@
> >      window.pktUI.pocketPanelDidHide(event);
> >    },
> >  
> > +  onCreated(node) {
> > +    node.setAttribute("disabled", Pocket.shouldDisable(node));
> 
> Won't onLocationChange end up re-enabling the button? Seems like it would
> need a shouldDisable check too.

onLocationChange was removed by bug 1164942.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/2dfd144f0bfd

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8605511 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/regressing bug #]: pocket
[User impact if declined]: attempts to use Pocket in private browsing windows will fail because it will log in through private cookies but the button references non-private cookies
[Describe test coverage new/current, TreeHerder]: manual testing
[Risks and why]: not much expected, changes limited to pocket
[String/UUID change made/needed]: none

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/da500f6c2df2

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/2dfd144f0bfd
https://hg.mozilla.org/mozilla-central/rev/da500f6c2df2

Comment 17

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8605511 [details] [diff] [review]
Patch

Should be in 38.0.5beta3.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/41157e1d9a86

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/f5828f333524

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/301797f68aa2

Comment 21

[Bogdan Maris, Desktop Test Engineering]

Reproduced the initial issue using Firefox 38.0.5 beta 1, verified that opening the view list from a temporary private window will open a new tab in normal window and will show the list of saved pages. Verification was done on Firefox 38.0.5 beta 3 across platforms (Windows 7 64-bit, Mac OS X 10.9.5 and Ubuntu 14.04 32-bit).

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #7)
> Created attachment 8605511 [details] [diff] [review]
> Patch
> 
> This patch fixes the case for temporary private browsing.
> 
> Permanent private browsing windows still don't work because pktApi.js uses
> nsICookieManager2.getCookiesFromHost which uses the non-private browsing
> cookie jar.
> 
> It looks like to get this working in private browsing mode, we will need to
> use nsICookieManager2.getCookiesForApp and pass in the Firefox app ID,
> preferably with a Pocket-related browser. Without the specific browser that
> will return *all* cookies for the app in the cookie jar.

Is there a followup bug on fixing this issue in permanent private mode?

Comment 22

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #21)
> Is there a followup bug on fixing this issue in permanent private mode?

Not that I know of, can you file one?

Comment 23

[Bogdan Maris, Desktop Test Engineering]

Also verified that this issue is fixed using Firefox 39 beta 1 build 2 across platforms (Windows 7 64-bit, Mac OS X 10.9.5 and Ubuntu 14.04 32-bit).

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #22)
> (In reply to Bogdan Maris, QA [:bogdan_maris] from comment #21)
> > Is there a followup bug on fixing this issue in permanent private mode?
> 
> Not that I know of, can you file one?

Filed bug 1168752 on that.

Comment 24

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Removing qe-verify+ as it seems there is no more manual testing needed here.