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

RESOLVED FIXED in mozilla25

Status

()

Toolkit
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: adw, Assigned: adw)

Tracking

Trunk
mozilla25
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
Created attachment 772398 [details] [diff] [review]
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.
Attachment #772398 - Flags: review?(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?
(Assignee)

Comment 2

5 years ago
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.
Attachment #772398 - Flags: review?(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.
(Assignee)

Comment 4

5 years ago
Yes, I suck at logical thinking.
(Assignee)

Comment 5

5 years ago
Created attachment 772947 [details] [diff] [review]
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.
Attachment #772398 - Attachment is obsolete: true
Attachment #772947 - Flags: review?(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.
Attachment #772947 - Flags: review?(mhammond) → review+
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
(Assignee)

Updated

5 years ago
Depends on: 893404
(Assignee)

Comment 9

5 years ago
Created attachment 776027 [details] [diff] [review]
patch 3

Captures should finish asynchronously when there are private windows open.  See bug 893404 comment 3.  I'll land this later today.
Attachment #772947 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/4fd5a8c1d7b2
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.