891169 - BackgroundPageThumbs should check private browsing status when a capture starts, not when it's requested

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Drew Willcoxon :adw]

Attachment: patch

BackgroundPageThumbs discards a capture request if there are private windows open when it's made.  But by the time the request would have been dequeued and serviced, there may no longer be any private windows open.  Similarly, even if there are no private windows open when the request is made, there may be when it's serviced.

Capture.start should do the private window check, not BackgroundPageThumbs.capture.  This builds on your recent patches, Mark.

Comment 1

[Mark Hammond [:markh] [:mhammond]]

If we want to make this bullet-proof, I'd think we need a way to detect that PB was entered at any point during the capture, and if so, just discard it at the end.  It seems there is a (very low, but real) risk that a "slow" capture might start and end with no PB windows open, but with a PB window having been open at a critical part of the capture process such that private data is still captured?

Comment 2

[Drew Willcoxon :adw]

Comment on attachment 772398 [details] [diff] [review]
patch

Good point!  Hmm.  What do you think about comparing timestamps?  Time stamp the page load start, page load end, and the lifetimes of private windows.  Check if they overlap at page load end, and if they do, cancel the capture.  Handling IPC might be tricky, though.

Comment 3

[Mark Hammond [:markh] [:mhammond]]

I'd have thought a simpler way would be a window watcher that just sets a flag to true would be fine?  So, something like:

* add observer that sets the flag.
* enumerate all windows, any existing PB then bail (ie, remove observer)
* start capture, and when complete...
* if the flag is true, discard data and requeue the url.

I think the chances of this actually happening are small enough that there's no real need to optimize it - ie, no need to check as the 'load' event fires - just have the the child send the captured data back and have the parent discard it.  That's possibly going to cause a false-positive or 2 in theory (eg, PB-mode being entered after load but before the parent gets the binary capture), but I think that's perfectly fine too.

Comment 4

[Drew Willcoxon :adw]

Yes, I suck at logical thinking.

Comment 5

[Drew Willcoxon :adw]

Attachment: patch 2

This does what you suggest except for requeueing the URL.  I'm not sure that's necessary, and we'd have to cap the number of times a capture is requeued somehow, or do exponential back-off or something.

Comment 6

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 772947 [details] [diff] [review]
patch 2

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

Totally agree re the requeue thing!

::: toolkit/components/thumbnails/BackgroundPageThumbs.jsm
@@ +259,5 @@
>      this._msgMan.addMessageListener("BackgroundPageThumbs:didCapture", this);
> +
> +    // Observe private windows: if a private window opens during the capture,
> +    // discard the capture when it finishes.
> +    Services.ww.registerNotification(this);

although it looks safe currently, it might be better to stick this at the top of the function, so the observer is added before the explicit check.  The (slight) concern is that at some point in the future new code gets added which silently spins the event loop, which could cause the new private window to be created in between the explicit check and the add of the observer.

Comment 7

[Drew Willcoxon :adw]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1b368df20595

Comment 8

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/de8a514dcdc5 because I didn't know whether it was this or bug 870104 that caused the oddly 10.8 opt only timeouts like https://tbpl.mozilla.org/php/getParsedLog.php?id=25256458&tree=Mozilla-Inbound

Comment 9

[Drew Willcoxon :adw]

Attachment: patch 3

Captures should finish asynchronously when there are private windows open.  See bug 893404 comment 3.  I'll land this later today.

Comment 10

[Drew Willcoxon :adw]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4fd5a8c1d7b2

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4fd5a8c1d7b2