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.

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

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.

Yes, I suck at logical thinking.

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

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

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