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

Status: RESOLVED FIXED

(DevTools :: Console, defect)

Description

[Josh Matthews [:jdm]]

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.

Comment 1

[Girish Sharma [:Optimizer]]

Attachment: WIP 1

WIP, failing one test that needs help from Ehsan :)

Comment 2

[Girish Sharma [:Optimizer]]

@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.

Comment 3

[Josh Matthews [:jdm]]

If you set browser.privatebrowsing.keep_current_session to true, none of the transition code needs to be present.

Comment 4

[Girish Sharma [:Optimizer]]

(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 ?

Comment 5

[(no longer active)]

(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.

Comment 6

[(no longer active)]

Also, how did we not catch this before?  This should assert fatally in debug builds...

Comment 7

[Josh Matthews [:jdm]]

What do you mean? A null context means that it's treated as non-private on the clipboard.

Comment 8

[(no longer active)]

(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.

Comment 9

[Girish Sharma [:Optimizer]]

Attachment: Patch v1.0 with tests

Pushed to try : 
https://tbpl.mozilla.org/?tree=Try&rev=2fa82e3123cd

Comment 10

[Girish Sharma [:Optimizer]]

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 11

[Josh Matthews [:jdm]]

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?

Comment 12

[Girish Sharma [:Optimizer]]

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 13

[Joe Walker [:jwalker]]

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.

Comment 14

[Josh Matthews [:jdm]]

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.

Comment 15

[Girish Sharma [:Optimizer]]

Attachment: patch v1.1

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.

Comment 16

[Girish Sharma [:Optimizer]]

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 ?

Comment 17

[Girish Sharma [:Optimizer]]

Attachment: patch using execute soon

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.

Comment 18

[Josh Matthews [:jdm]]

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?

Comment 19

[Girish Sharma [:Optimizer]]

Attachment: patch using execute soon v2

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).

Comment 20

[Girish Sharma [:Optimizer]]

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.

Comment 21

[Joe Walker [:jwalker]]

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.

Comment 22

[Girish Sharma [:Optimizer]]

Attachment: Patch with async tests

Updated the test to be asynchronous.

Try is green at https://tbpl.mozilla.org/?tree=Try&rev=cecd5e792e66

Comment 23

[Josh Matthews [:jdm]]

Comment on attachment 663663 [details] [diff] [review]
Patch with async tests

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

Nice!

Comment 24

[Girish Sharma [:Optimizer]]

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

Comment 25

[Paul Rouget [:paul]]

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

Comment 26

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/c4397c10018b