Closed Bug 792533 Opened 7 years ago Closed 7 years ago

gcli screenshot command provides a null privacy context initializer to the clipboard transferable

Categories

(DevTools :: Console, defect)

x86_64
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 18

People

(Reporter: jdm, Assigned: Optimizer)

References

Details

Attachments

(1 file, 5 obsolete files)

This means that if you take a screenshot of a window in private browsing mode, the image will stay on the clipboard after you exit out of it. The argument to init() is present to avoid this exact situation, and must be non-null in this context.
Attached patch WIP 1 (obsolete) — Splinter Review
WIP, failing one test that needs help from Ehsan :)
Assignee: nobody → scrapmachines
Status: NEW → ASSIGNED
@Ehsan, one note:
I tested the same manually using Scratchpad, changing the private browsing mode just like in the test.
I was able to see that after switching from PB to non PB mode, the clipboard was only emptied after the gBrowser.selectedTab.linkedBrowser's 'load' has happened.

So if you remove the commented lines that do the same, you should have all test passing, except for the fact that when I do that, the methods 'ok' and 'is' are not available.
If you set browser.privatebrowsing.keep_current_session to true, none of the transition code needs to be present.
(In reply to Josh Matthews [:jdm] from comment #3)
> If you set browser.privatebrowsing.keep_current_session to true, none of the
> transition code needs to be present.

So I can simply do everything in sync ?
(In reply to Girish Sharma [:Optimizer] from comment #4)
> (In reply to Josh Matthews [:jdm] from comment #3)
> > If you set browser.privatebrowsing.keep_current_session to true, none of the
> > transition code needs to be present.
> 
> So I can simply do everything in sync ?

Yes.
Also, how did we not catch this before?  This should assert fatally in debug builds...
What do you mean? A null context means that it's treated as non-private on the clipboard.
(In reply to comment #7)
> What do you mean? A null context means that it's treated as non-private on the
> clipboard.

Well, shut my mouth please!  I was just not paying attention.
Attached patch Patch v1.0 with tests (obsolete) — Splinter Review
Pushed to try : 
https://tbpl.mozilla.org/?tree=Try&rev=2fa82e3123cd
Attachment #662995 - Attachment is obsolete: true
Attachment #663075 - Flags: review?(jwalker)
Attachment #663075 - Flags: review?(josh)
Ah, I forgot to mention some important things:

1. The way the ok command was checking things in the original test was wrong. My bad. ok command does not execute the function passed to it. I have fixed that now.

2. Added a max-height to the screenshot so that when you do a fullpage screenshot, the preview does not take over your screen.

3. Added quotes around the file name.
Comment on attachment 663075 [details] [diff] [review]
Patch v1.0 with tests

This looks good to me, modulo my limited understanding of the screenshot tool. Can you take a screenshot of just a content window, or part of one? All of the loadContexts you are using seem to be from chrome windows; is there any case in which you should be using the content window object instead?
I tried using the content window to get the loadContext, but that was not respecting private browsing, and my screenshot was still available outside of private browsing mode. So I shifted to always use chrome window for getting the loadContext
Comment on attachment 663075 [details] [diff] [review]
Patch v1.0 with tests

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

r+ for the gcli side, I would like someone to review this that understands private browsing though.
Thanks Optimizer

::: browser/devtools/commandline/CmdScreenshot.jsm
@@ +82,4 @@
>      if (args.delay > 0) {
>        var promise = context.createPromise();
>        document.defaultView.setTimeout(function Command_screenshotDelay() {
> +        let reply = 

I'll leave it up to you to decide if this is worth it, but you added a trailing space to the end of the line here.
Attachment #663075 - Flags: review?(jwalker) → review+
That sounds like that warrants further investigations. The privacy status is supposed to flow down from the top level, so if the chrome window is reporting private, I would expect any child element with a load context below that to report the same.
Attached patch patch v1.1 (obsolete) — Splinter Review
So here is what actually was happening:
I tested too early using the contentWindow.

The real problem is not content or chrome window, but the fact that when you switch out of private browsing mode, the clipboard remains available until the browser is fully loaded. i.e all pinned tabs + active tab (or all if you have the setting to load on demand off) are loaded.

This, imo , is a security concern.

Anyways, I have updated the patch to use proper window.
Attachment #663075 - Attachment is obsolete: true
Attachment #663075 - Flags: review?(josh)
Attachment #663112 - Flags: review?(josh)
And I guess, due to the same fact, the one check might fail, depending upon how fast clipboard is emptied (as seen on OSX oth).

@Josh, should I add an event handler to wait for load finish, and then check ?
Attached patch patch using execute soon (obsolete) — Splinter Review
As of now only OSX 10.8 oth is failing the test that checks clipboard after leaving private browsing.

I cannot add an event listener as 'ok' and 'is' methods are not available to me, so adding executeSoon in this patch.
Rest is all similar to the previous patch.
I feel like the fact that executeSoon works at all is a fluke - you're doing asynchronous things after the otherwise synchronous test has finished, aren't you? What if you throw a waitUntilExplicitFinish in there and finish in the event handler?
Attached patch patch using execute soon v2 (obsolete) — Splinter Review
Now using waitForExplicitFinish, and finish calls.

In my local machine, all tests pass whether I use executeSoon or not. I depends on how soon the clipboard is emptied, so to be sure, I am preferring to use executeSoon.

I would have gone with eventListeners, but inside them, oka nd is are not defined (dunno why).
Attachment #663112 - Attachment is obsolete: true
Attachment #663117 - Attachment is obsolete: true
Attachment #663112 - Flags: review?(josh)
Attachment #663124 - Flags: review?(josh)
The try results are not good. This test is in need for async tests, for the file creation as well as clipboard part of the test.

@Joe, is it possible to have async tests? I tried putting in some event handlers, but ok,is,info were not available inside them.
The problem isn't that ok,is,etc are not available, it's that the page has gone away when you're trying to execute the event handler.

The test() function has a list of tests that it needs to run, and it waits until they're done, so we don't need to do waitForExplicitFinish();

What we do need to do is to add our event hander to the list of things that must happen before the test is done:

var handler = DeveloperToolbarTest.checkCalled(function(ev) {
  ok(....);
});

One thing that's important (right now) is that checkCalled is called after DeveloperToolbarTest.test(), so it's best placed in one of the functions listed in the call to test.

See browser_cmd_addon.js for an example.
Updated the test to be asynchronous.

Try is green at https://tbpl.mozilla.org/?tree=Try&rev=cecd5e792e66
Attachment #663124 - Attachment is obsolete: true
Attachment #663124 - Flags: review?(josh)
Attachment #663663 - Flags: review?(jwalker)
Attachment #663663 - Flags: review?(josh)
Comment on attachment 663663 [details] [diff] [review]
Patch with async tests

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

Nice!
Attachment #663663 - Flags: review?(josh) → review+
Attachment #663663 - Flags: review?(jwalker) → review+
Whiteboard: [land-in-fx-team]
Can anyone please land this one too ?
It has been hanging around for 3 days and many things have landed in between to fx-team :)
I want to avoid rabasing it :P
https://hg.mozilla.org/integration/fx-team/rev/c4397c10018b
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/c4397c10018b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 18
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.