Closed
Bug 816257
Opened 12 years ago
Closed 11 years ago
Handling panels with per-window private browsing
Categories
(Add-on SDK Graveyard :: General, defect, P1)
Add-on SDK Graveyard
General
Tracking
(firefox21 fixed, firefox22+ fixed, firefox23 fixed)
RESOLVED
FIXED
mozilla23
People
(Reporter: evold, Unassigned)
References
Details
Attachments
(3 files)
355 bytes,
text/html
|
ochameau
:
review+
|
Details |
355 bytes,
text/html
|
irakli
:
review+
|
Details |
3.59 KB,
patch
|
mossop
:
review+
bajaj
:
approval-mozilla-aurora+
bajaj
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•12 years ago
|
||
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.
Comment 2•12 years ago
|
||
(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.
Reporter | ||
Comment 3•12 years ago
|
||
(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?
Reporter | ||
Comment 4•12 years ago
|
||
(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?
Comment 5•12 years ago
|
||
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.
Comment 6•12 years ago
|
||
(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.
Comment 7•12 years ago
|
||
(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...) :/
Comment 8•12 years ago
|
||
(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?
Reporter | ||
Updated•12 years ago
|
Comment 9•12 years ago
|
||
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
Reporter | ||
Comment 10•12 years ago
|
||
(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.
Reporter | ||
Comment 11•12 years ago
|
||
(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.
Comment 12•12 years ago
|
||
OK, so should I write that patch here?
Reporter | ||
Comment 13•12 years ago
|
||
(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.
Reporter | ||
Updated•12 years ago
|
Assignee: nobody → evold
Reporter | ||
Updated•12 years ago
|
Comment 14•12 years ago
|
||
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.
Comment 15•12 years ago
|
||
We need to decide on what we want to do with regard to handling of panels here soon.
Flags: needinfo?(evold)
Reporter | ||
Comment 16•12 years ago
|
||
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)
Comment 17•11 years ago
|
||
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!
Comment 18•11 years ago
|
||
Making this not block bug 463027 directly. Bug 748604 handles all we need to track there.
No longer blocks: PBnGen
Reporter | ||
Updated•11 years ago
|
Blocks: sdk-pwpb-fx21
Reporter | ||
Updated•11 years ago
|
Target Milestone: --- → 1.14
Reporter | ||
Updated•11 years ago
|
No longer blocks: sdk-pwpb-fx21
Reporter | ||
Comment 19•11 years ago
|
||
panels won't be supported in private windows until 1.15 now..
Target Milestone: 1.14 → 1.15
Reporter | ||
Updated•11 years ago
|
Assignee: evold → rFobic
This needs to get uplifted to at least Aurora, once it is written.
Comment 21•11 years ago
|
||
(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.
tracking-firefox22:
--- → ?
Target Milestone: 1.15 → ---
Updated•11 years ago
|
QA Contact: rFobic
Comment 22•11 years ago
|
||
Pointer to Github pull-request
Updated•11 years ago
|
Attachment #734179 -
Flags: review?(poirot.alex)
Comment 23•11 years ago
|
||
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 24•11 years ago
|
||
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+
Comment 25•11 years ago
|
||
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
Updated•11 years ago
|
Comment 26•11 years ago
|
||
(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?
Updated•11 years ago
|
status-firefox22:
--- → affected
status-firefox23:
--- → affected
Reporter | ||
Comment 27•11 years ago
|
||
We're planning on allowing a single panel to travel across windows regardless of it's private browsing state now.
Reporter | ||
Comment 28•11 years ago
|
||
Pointer to Github pull-request
Reporter | ||
Updated•11 years ago
|
Attachment #738718 -
Flags: review?(rFobic)
Updated•11 years ago
|
Attachment #738718 -
Flags: review?(rFobic) → review+
Comment 29•11 years ago
|
||
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
Reporter | ||
Updated•11 years ago
|
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
Assignee: rFobic → evold
QA Contact: rFobic
Comment 30•11 years ago
|
||
Attachment #741477 -
Flags: review+
Comment 31•11 years ago
|
||
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
Updated•11 years ago
|
Product: Add-on SDK → Firefox
Comment 32•11 years ago
|
||
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?
Updated•11 years ago
|
Flags: approval-mozilla-beta?
Flags: approval-mozilla-aurora?
Product: Firefox → Add-on SDK
Target Milestone: --- → mozilla23
Updated•11 years ago
|
Attachment #741477 -
Flags: approval-mozilla-beta?
Attachment #741477 -
Flags: approval-mozilla-aurora?
Comment 33•11 years ago
|
||
(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 34•11 years ago
|
||
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+
Comment 35•11 years ago
|
||
(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 36•11 years ago
|
||
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+
Comment 37•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/e494ef9322a4 https://hg.mozilla.org/releases/mozilla-beta/rev/8a55373a9363
Comment 38•11 years ago
|
||
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
Comment 39•11 years ago
|
||
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
Comment 40•11 years ago
|
||
Why do not Addon SDK repository integrate into mercurial? It is impossible to pinpoint the regression cset.
Reporter | ||
Updated•11 years ago
|
Assignee: evold → nobody
You need to log in
before you can comment on or make changes to this bug.
Description
•