1256362 - Hello breaks private browsing

Status: VERIFIED FIXED

(Hello (Loop) :: Client, defect, P1)

Description

[(no longer active)]

Attachment: Screenshot

I tested this in Firefox 45.

STR:

1. Open a private and a non-private window.
2. In the private window, go to a website that is not otherwise saved in your history.
3. Click Tools > Start a conversation (which is *not* disabled, contrary to the toolbar icon.)
4. Click "Browse this page with a friend"
5. Restart, and click on the Hello icon on a normal window.

Your private session is saved and recorded.

Comment 1

[Mark Banner (:standard8)]

Given that all we do is open up a panel with a browser element, I'm a bit concerned that (especially from the add-on perspective) something this simple makes private browsing fail.

We can disable/remove the menu option in private browsing mode, but there seems to be a bigger issue with private browsing.

Comment 2

[GitHub Autolander Bot]

Attachment: [loop] Standard8:bug-1256362-private > mozilla:master

Comment 3

[Mark Banner (:standard8)]

Comment on attachment 8731661 [details] [review]
[loop] Standard8:bug-1256362-private > mozilla:master

Sevaan: I've gone for removing the menu item rather than disabling it. I think that's standard practice, and the button on the toolbar is disabled (with its own tooltip), so a user should be able to work out why.

Is that ok with you?

Comment 5

[Manuel Casas Barrado [:mancas]]

Comment on attachment 8731661 [details] [review]
[loop] Standard8:bug-1256362-private > mozilla:master

r=me. Looks good. I left a comment on the PR

Comment 6

[(no longer active)]

(In reply to Mark Banner (:standard8) from comment #1)
> Given that all we do is open up a panel with a browser element, I'm a bit
> concerned that (especially from the add-on perspective) something this
> simple makes private browsing fail.

It's not opening the panel that breaks anything.  It's Hello saving the history without checking whether the tab is in private browsing mode.

Private browsing works by every component that writes anything about the user's history to disk checking to make sure the source of the data is not a private window.  There is no magic that prevents some add-on from doing that.

> We can disable/remove the menu option in private browsing mode, but there
> seems to be a bigger issue with private browsing.

There's no bigger issues here, just that Hello wasn't doing what it should.  :-)

As far as the fix goes, if this actually prevents the code that saves the history from accessing any private window under *all* circumstances, then that's a sufficient fix.  I don't know the Hello code well enough to know whether disabling the menu option is enough.  There may be other types of scenarios to think about, for example, when you pop a detached Hello panel back into a window, does anything check to make sure the target window is not private?  When tab sharing is active, does anything check whether the tab that the user switches to is in private mode?  etc.

Comment 7

[Mark Banner (:standard8)]

https://github.com/mozilla/loop/commit/9eae8f2c749d16c44f0422fb400d95b03a96d095

Comment 8

[Mark Banner (:standard8)]

Comment 6 discussion is taking place in bug 1257487.

This is planned to roll out in the next version of the add-on - 1.2 which is planned to go to 46.

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking, we want to make sure private browsing works properly with Hello.

Comment 10

[Mark Banner (:standard8)]

This is rolling out in 1.2.2 (bug 1258865).

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

This would also be good to verify once it lands (either in beta 6 build 2, or in beta 7)

Comment 12

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

I managed to reproduce this bug on 45.0.2 build1 (20160407164938) using:
- Windows 10 x64, 
- Windows 8.1 x86, 
- Ubuntu 12.04 x64, 
- Mac OS X 10.8. 
Also I verified it on:
- 46.0b10 build1 (20160411042519), 
- 47.0a2 (2016-04-11),
- 48.0a1 (2016-04-12), 
using the above mentioned platforms.