Handling panels with per-window private browsing

RESOLVED FIXED in Firefox 21

Status

defect
P1
normal
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: evold, Unassigned)

Tracking

unspecified
mozilla23
Dependency tree / graph

Firefox Tracking Flags

(firefox21 fixed, firefox22+ fixed, firefox23 fixed)

Details

Attachments

(3 attachments)

The options I see are:

1 Don't display panels in PB-enabled windows (which means most apis that would use a panel also need to be disabled, like context-menu, otherwise they'd have to throw an error while trying to open a panel)
2 use appshell.privateHiddenWindow for panels always

I really don't like either option, am I missing something?

In order to move panels from window to window while preserving state it seems to me that we will need to be able to switch the pb-mode of the docShell.
from bug 816202

(In reply to Mark Hammond (:markh) from comment #8)
> (In reply to Erik Vold [:erikvold] [:ztatic] from comment #6)
> 
> > In order to move panels from window to window while preserving state it
> > seems to me that we will need to be able to switch the pb-mode of the
> > docShell.
> 
> My concern with that approach is that a single window could then correlate
> information between PB-mode and non PB-mode.  Eg, a website could store in
> memory the identity of the logged in user in both modes, which could
> possibly be sensitive to the user.
> 
> IOW, "while preserving state" is exactly what we don't want to allow when
> talking about flipping between PB and non-PB.

Hmm this is a good point, I'm starting to like Google Chrome's approach, disabling add-ons for pb-enabled windows by default and giving the user the option to use the add-on on pb-enabled windows.
(In reply to Erik Vold [:erikvold] [:ztatic] from comment #0)
> The options I see are:
> 
> 1 Don't display panels in PB-enabled windows (which means most apis that
> would use a panel also need to be disabled, like context-menu, otherwise
> they'd have to throw an error while trying to open a panel)

That sounds fine to me, but I don't know if it's acceptable for Jetpack.

> 2 use appshell.privateHiddenWindow for panels always

Why do we need to do that?  I was thinking about using the regular hiddenWindow for regular windows, and the privateHiddenWindow for private windows.
Blocks: 815847
(In reply to Ehsan Akhgari [:ehsan] from comment #2)
> (In reply to Erik Vold [:erikvold] [:ztatic] from comment #0)
> > 2 use appshell.privateHiddenWindow for panels always
> 
> Why do we need to do that?  I was thinking about using the regular
> hiddenWindow for regular windows, and the privateHiddenWindow for private
> windows.

Could you explain this idea a little more for me? I don't think that I fully understand.  Do you mean have two separate hidden windows for panels? One that is private and the other that is not, then store the panel frame in the appropriate hiddenWindow?
(In reply to Erik Vold [:erikvold] [:ztatic] from comment #3)
> Could you explain this idea a little more for me? I don't think that I fully
> understand.  Do you mean have two separate hidden windows for panels? One
> that is private and the other that is not, then store the panel frame in the
> appropriate hiddenWindow?

So there would be two versions of a panel instance, and two states for the panel? one that exists for private windows and the other that exists for non-private windows?
Not wanting to hijack this bug, but we'd be looking for something like this to enable the social components in PB mode - either 2 top-level hidden windows (one in each mode), or the ability to create 2 iframes in the existing hidden window but adjust the PB mode of one of them.

I can't recall exactly how Jetpack "parks" the panels, but maybe this could form the basis of a general approach - have the hidden window create an iframe specifically for PB mode, then Social could create a sub-iframe inside the designated PB one, and maybe Jetpack could also "park" its panels for PB mode there too.
(In reply to comment #3)
> (In reply to Ehsan Akhgari [:ehsan] from comment #2)
> > (In reply to Erik Vold [:erikvold] [:ztatic] from comment #0)
> > > 2 use appshell.privateHiddenWindow for panels always
> > 
> > Why do we need to do that?  I was thinking about using the regular
> > hiddenWindow for regular windows, and the privateHiddenWindow for private
> > windows.
> 
> Could you explain this idea a little more for me? I don't think that I fully
> understand.  Do you mean have two separate hidden windows for panels? One that
> is private and the other that is not, then store the panel frame in the
> appropriate hiddenWindow?

Yes, exactly.  However, depending on what the SDK panel API looks like (which I know nothing about), this may or may not be feasible.
(In reply to comment #5)
> Not wanting to hijack this bug, but we'd be looking for something like this to
> enable the social components in PB mode - either 2 top-level hidden windows
> (one in each mode), or the ability to create 2 iframes in the existing hidden
> window but adjust the PB mode of one of them.
> 
> I can't recall exactly how Jetpack "parks" the panels, but maybe this could
> form the basis of a general approach - have the hidden window create an iframe
> specifically for PB mode, then Social could create a sub-iframe inside the
> designated PB one, and maybe Jetpack could also "park" its panels for PB mode
> there too.

Just to be clear, which hidden window you use only matters if the thing that you do with it has privacy implications.  For example, if you use hiddenWindow.document.createElement to create DOM elements then chances are that you're fine.  If you use it to perform network requests (for example by embedding an iframe under that doc) then you're not...

I can write the patch to add a hiddenPrivateWindow very fast, I just want us to make sure that is what we really want (since that is a very sucky solution...) :/
(In reply to comment #4)
> (In reply to Erik Vold [:erikvold] [:ztatic] from comment #3)
> > Could you explain this idea a little more for me? I don't think that I fully
> > understand.  Do you mean have two separate hidden windows for panels? One
> > that is private and the other that is not, then store the panel frame in the
> > appropriate hiddenWindow?
> 
> So there would be two versions of a panel instance, and two states for the
> panel? one that exists for private windows and the other that exists for
> non-private windows?

Yes, that's the proposal.  Does that make sense from the SDK point of view?
Blocks: PBnGen
OS: Mac OS X → All
Hardware: x86 → All
TBH, I'm not sure how Jetpack can know how to do the right thing here.  A reasonable pattern is something like:

let panel = require("panel").Panel({
  contentURL: "https://www.facebook.com"
});

panel.show(anchor1);
... and later
panel.show(anchor2);
... even later
panel.show(); // no anchor this time

Consider if 'anchor1' is in a normal top-level window and 'anchor2' is in a PB top-level window.  Now consider the user is logged into one account in the main window, but a different account in the PB window.

From the user's POV, they'd expect to see one set of content when 'anchor1' is used and a completely different set of content when 'anchor2' is used - but it's the same panel object.  If there is no anchor specified, Jetpack will use getMostRecentWindow("navigator:browser"); - which could be either of them.

At face value, it sounds like Jetpack would need a significant change such that a single "Panel" instance is a kind of proxy for multiple XUL panel objects, automatically selecting the appropriate one after the choice of window is made (based on the anchor or getMostRecentWindow), which itself would have serious implications for the developer of the addon (eg, attributes etc set on the panel object while 'anchor1' is used might not be reflected once 'anchor2' is used)

I assume the same basic complication would exist for page-mods etc too.
Priority: -- → P1
(In reply to Mark Hammond (:markh) from comment #9)
> At face value, it sounds like Jetpack would need a significant change such
> that a single "Panel" instance is a kind of proxy for multiple XUL panel
> objects, automatically selecting the appropriate one after the choice of
> window is made (based on the anchor or getMostRecentWindow), which itself
> would have serious implications for the developer of the addon (eg,
> attributes etc set on the panel object while 'anchor1' is used might not be
> reflected once 'anchor2' is used)

If we went for the proxy method that you describe then that could break how some add-ons work, assuming that those add-ons are keeping track of the panel's state from outside the panel, which I'm sure some are doing.  This might mean that jetpacks using panels will need to be re-reviewed.

> I assume the same basic complication would exist for page-mods etc too.

Yes, widgets, page-mods, context-menus instances are all proxies already.  However jetpacks reviewed today are expected to use our 'private-browsing' module if they want to write anything to disk, and this module has a boolean property called `isActive` which in the pwpb world will be true if any open window is in pb mode, so afaict I think that our other modules wouldn't need modification or re-review.  I need to double check that widgets use separate iframes for browser windows tho.
(In reply to Ehsan Akhgari [:ehsan] from comment #7)
> (In reply to comment #5)
> > Not wanting to hijack this bug, but we'd be looking for something like this to
> > enable the social components in PB mode - either 2 top-level hidden windows
> > (one in each mode), or the ability to create 2 iframes in the existing hidden
> > window but adjust the PB mode of one of them.
> > 
> > I can't recall exactly how Jetpack "parks" the panels, but maybe this could
> > form the basis of a general approach - have the hidden window create an iframe
> > specifically for PB mode, then Social could create a sub-iframe inside the
> > designated PB one, and maybe Jetpack could also "park" its panels for PB mode
> > there too.
> 
> Just to be clear, which hidden window you use only matters if the thing that
> you do with it has privacy implications.  For example, if you use
> hiddenWindow.document.createElement to create DOM elements then chances are
> that you're fine.  If you use it to perform network requests (for example by
> embedding an iframe under that doc) then you're not...
> 
> I can write the patch to add a hiddenPrivateWindow very fast, I just want us
> to make sure that is what we really want (since that is a very sucky
> solution...) :/

Well I've thought about this a lot, and I think your suggestion here is probably the least sucky that I see, so let's go with this route.
OK, so should I write that patch here?
(In reply to Ehsan Akhgari [:ehsan] from comment #12)
> OK, so should I write that patch here?

I need a patch from you for bug 815847, and this one is for the sdk panel module updates which I will do.
Assignee: nobody → evold
Depends on: 820582
No longer blocks: 815854
Depends on: 815884, 815854
It looks like bug 815847 and the alternatives might not land soon.  Off the top of my head, I *think* it might be possible to make a short-term not-perfect "fix" for this which can be revisited.

Please excuse my hand-waving :)

What I'm thinking is that instead of "parking" the iframe in the hidden window, the iframe gets parked in the current "top-level" window.  If the top-level window where the iframe did exist is destroyed, then it gets recreated - which means the iframe state gets lost, but I'm thinking this might be acceptable short-term.  The reason it might be acceptable is:

* I doubt all that many jetpacks require the state to be kept anyway.  What often seems to happen is that the panel reflects the state in the current tab.  If you switch to a new window (or even a new tab), often the old panel state isn't relevant - the panel contents gets rebuilt by the addon based on the contents of the current tab.

* The state would only get discarded when the top-level window that currently owns the iframe gets destroyed.

* Panel state is always lost on restart anyway, so nothing is going to get seriously broken by losing state, just inconvenienced.

The combination of the above points makes me believe it will be relatively rare that the panel state gets lost when it matters, and thus might be a better short-term solution than doing nothing at all while we wait for a better fix.  It *might* even be possible to swapFrameLoaders when the top-level window is being destroyed and park it in some other top-level window, meaning state *never* need get lost.

So what this would mean is that:

* Jetpack ends up tracking 2 iframes per panel - one for normal windows, one for private windows.  When the panel gets hidden, the frame just gets parked in whatever top-level window with the required privacy settings was most recently used.  When the panel is needed again, it just tries to grab it from the recent top-level window that matches the current privacy settings and attempts to reuse it.  If it finds the frame no longer exists, a new frame gets created and this new frame ends up parked in the new window.

End result is that the panel *never* gets reset while used repeatedly in the same top-level window (presumably the most common case) but still magically works in other cases, albiet with old state lost.
We need to decide on what we want to do with regard to handling of panels here soon.
Flags: needinfo?(evold)
I think we are going to have to break the panel API by using multiple instances, some which are now private and expire when all pb windows are closed.
Flags: needinfo?(evold)
I just landed the patches for bug 815847 on mozilla-inbound, sorry that it took a bit more than we were expecting.  Those patches add hiddenPrivateWindow and hiddenPrivateDOMWindow.  Please let me know if you need anything else on the platform side for the purposes of this bug.  Thanks!
No longer blocks: 815847
Depends on: 815847
Making this not block bug 463027 directly.  Bug 748604 handles all we need to track there.
No longer blocks: PBnGen
Target Milestone: --- → 1.14
Depends on: 847776
Depends on: 847771
Depends on: 849723
No longer blocks: sdk-pwpb-fx21
panels won't be supported in private windows until 1.15 now..
Target Milestone: 1.14 → 1.15
Assignee: evold → rFobic
This needs to get uplifted to at least Aurora, once it is written.
(In reply to Wes Kocher (:KWierso) from comment #20)
> This needs to get uplifted to at least Aurora, once it is written.

Requesting tracking-firefox22 so that it's on someone's radar. Once this lands on github and is greaan, we'll submit to m-i and then submit a patch for m-a.
Target Milestone: 1.15 → ---
Comment on attachment 734179 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/928

The number of comments is relative to the size of the changes,
but the patch looks overall good.
But it seems to break anchors. On reddit-panel example, the panel blinks and isn't anchored to the widget.

Otherwise two things:
- have you tested that panel doesn't leak?
- I'm really concerned by the usage of 'utils' modules everywhere.
It is misleading in multiple ways. And it term of code layout, it is really easy to end up with big files with random functions, that only few people knows about. And on top of that, having 'helpers' modules, which tend to be very similar, doesn't help.
Whereas in some cases, like this one, this utils module could have been named View, DOM, or something else that makes it more explicit and limit the scope of this module.
Attachment #734179 - Flags: review?(poirot.alex) → review-
Comment on attachment 734179 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/928

Looks good with the new revisions, thanks!
Attachment #734179 - Flags: review- → review+
Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/9bb40faeecf1de3974ff6f16625ea23d3d66e9af
Merge pull request #928 from Gozala/bug/private-panels@816257

Bug 816257 - Detraitify panel implementation. r=@ochameau
(In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from comment #22)
> Created attachment 734179 [details]
> Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/928
> 
> Pointer to Github pull-request

When do we expect this to land on m-c in preparation for m-a?
We're planning on allowing a single panel to travel across windows regardless of it's private browsing state now.
Attachment #738718 - Flags: review?(rFobic)
Attachment #738718 - Flags: review?(rFobic) → review+
Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/e1f07ae3a266e9b274509b3001937413c6a3d672
Bug 816257: allowing non-private panel windows in private browsing windows now..

https://github.com/mozilla/addon-sdk/commit/c5b6fdb532032ee4ac257b395fcd2b475fea2c63
Merge pull request #950 from erikvold/816257

Bug 816257: allowing non-private panel windows in private browsing windows now r=@gozala
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Assignee: rFobic → evold
QA Contact: rFobic
Commits pushed to integration at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/e1f07ae3a266e9b274509b3001937413c6a3d672
Bug 816257: allowing non-private panel windows in private browsing windows now..

https://github.com/mozilla/addon-sdk/commit/c5b6fdb532032ee4ac257b395fcd2b475fea2c63
Merge pull request #950 from erikvold/816257
Product: Add-on SDK → Firefox
Comment on attachment 741477 [details] [diff] [review]
Patch for beta

[Approval Request Comment]
Bug caused by (feature/regressing bug #): per-window private browsing
User impact if declined: Add-on developers can't use pop-up panels in Firefox 21 or 22
Testing completed (on m-c, etc.): In nightlies for a few days
Risk to taking this patch (and alternatives if risky): Zero risk to the app, Firefox doesn't use this code. It is also very simple code, mostly tests.
String or IDL/UUID changes made by this patch: None
Attachment #741477 - Flags: approval-mozilla-beta?
Attachment #741477 - Flags: approval-mozilla-aurora?
Flags: approval-mozilla-beta?
Flags: approval-mozilla-aurora?
Product: Firefox → Add-on SDK
Target Milestone: --- → mozilla23
Attachment #741477 - Flags: approval-mozilla-beta?
Attachment #741477 - Flags: approval-mozilla-aurora?
(In reply to Dave Townsend (:Mossop) from comment #32)
> Comment on attachment 741477 [details] [diff] [review]
> Patch for beta
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): per-window private browsing
> User impact if declined: Add-on developers can't use pop-up panels in
> Firefox 21 or 22
> Testing completed (on m-c, etc.): In nightlies for a few days
> Risk to taking this patch (and alternatives if risky): Zero risk to the app,
> Firefox doesn't use this code. It is also very simple code, mostly tests.
> String or IDL/UUID changes made by this patch: None

based on my read of the comments(comment#20,#21) and tracking flags it seems right to uplift this to aurora.Why is it being nominated for beta?"per-window private browsing" landed in Fx-20 so Fx21 would then be in the same state as that, correct ? Am I mis-reading the need of this in our final beta's ?
Comment on attachment 741477 [details] [diff] [review]
Patch for beta

Approving this only on aurora based on comment 23.Please ping me or comment here is there is a disagreement on beta uplift asap .
Attachment #741477 - Flags: approval-mozilla-beta?
Attachment #741477 - Flags: approval-mozilla-beta-
Attachment #741477 - Flags: approval-mozilla-aurora?
Attachment #741477 - Flags: approval-mozilla-aurora+
(In reply to bhavana bajaj [:bajaj] from comment #33)
> (In reply to Dave Townsend (:Mossop) from comment #32)
> > Comment on attachment 741477 [details] [diff] [review]
> > Patch for beta
> > 
> > [Approval Request Comment]
> > Bug caused by (feature/regressing bug #): per-window private browsing
> > User impact if declined: Add-on developers can't use pop-up panels in
> > Firefox 21 or 22
> > Testing completed (on m-c, etc.): In nightlies for a few days
> > Risk to taking this patch (and alternatives if risky): Zero risk to the app,
> > Firefox doesn't use this code. It is also very simple code, mostly tests.
> > String or IDL/UUID changes made by this patch: None
> 
> based on my read of the comments(comment#20,#21) and tracking flags it seems
> right to uplift this to aurora.Why is it being nominated for
> beta?"per-window private browsing" landed in Fx-20 so Fx21 would then be in
> the same state as that, correct ? Am I mis-reading the need of this in our
> final beta's ?

Per-window private browsing landed in Firefox 20, but the add-on SDK wasn't updated in time to support it. Any add-ons wanting to support private browsing must use the version of the SDK targetted at and that shipped with Firefox 21, however because we hadn't got the implementation for panels right we disabled them.

So add-ons built with the SDK who want to support private browsing and panels won't work in Firefox 21 unless we take this fix there.
Comment on attachment 741477 [details] [diff] [review]
Patch for beta

Approving based on the recent comment #35.
Attachment #741477 - Flags: approval-mozilla-beta- → approval-mozilla-beta+
Commit pushed to firefox21 at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/34bf26c11c00d5cd624b0a0d45e9f565ca0ebe11
Bug 816257: Enable panels in per-window private browsing. r=gozala, a=bajaj
Commit pushed to firefox22 at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/c2b95c2d9267b7207e05aac0ad9de2e9034f57b0
Bug 816257: Enable panels in per-window private browsing. r=gozala, a=bajaj
Depends on: 886329
Why do not Addon SDK repository integrate into mercurial?
It is impossible to pinpoint the regression cset.
Assignee: evold → nobody
You need to log in before you can comment on or make changes to this bug.