Bug 94888

Created attachment 160307 [details] Patch

This patch works by forcing the plugin into private browsing mode when it's in a third-party context. I'm a bit concerned that it might not work exactly as desired because I'm not sure if it correctly handles private browsing mode changing. The patch only applies for WK2 currently, as WebKit 1 doesn't have an API for this currently.

I don't know how private browsing mode works for plugins, but does this create a leak between normal and private modes? For example, if I wanted to learn what someone was up to in private browsing mode, could I put a plugin in a third-party context and then get access to that information?

(In reply to comment #3) > I don't know how private browsing mode works for plugins, but does this create a leak between normal and private modes? For example, if I wanted to learn what someone was up to in private browsing mode, could I put a plugin in a third-party context and then get access to that information? I talked to people who know more than me about how plugins work, and they say that this shouldn't be a concern. The private browsing context is only for the plugin and not the whole document, so document/page acts as if it's a normal (but third-party) context, and the plugin just can't read/write LSOs.

Comment on attachment 160307 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160307&action=review > Source/WebKit2/WebProcess/Plugins/PluginView.cpp:1265 > + if (frame()->document() && !frame()->document()->securityOrigin()->canAccessPluginStorage(frame()->document()->topDocument()->securityOrigin())) How can frame()->document() be 0? That shouldn't be possible. frame()->document()->topDocument() seems like needless indirection. You're going to the Document, which just goes back to the Frame. Why not just frame()->tree()->top()->document() ?

This patch is fine from my PoV, but I don't really know how isPrivateBrowsingEnabled is it's probably better if someone who knows about plug-ins does the official review.

(In reply to comment #5) > (From update of attachment 160307 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=160307&action=review > > > Source/WebKit2/WebProcess/Plugins/PluginView.cpp:1265 > > + if (frame()->document() && !frame()->document()->securityOrigin()->canAccessPluginStorage(frame()->document()->topDocument()->securityOrigin())) > > How can frame()->document() be 0? That shouldn't be possible. I didn't know if it was possible, so I added that check to be safe (I thought it was, but I guess not). > frame()->document()->topDocument() seems like needless indirection. You're going to the Document, which just goes back to the Frame. Why not just frame()->tree()->top()->document() ? I also didn't realize that you could just do this. I've been using topDocument() in all other locations, so I just did that here too. That might work better.

> I didn't know if it was possible, so I added that check to be safe (I thought it was, but I guess not). It used to be possible, but it's not possible anymore. Most code that null checks the document is doing so needlessly. > > frame()->document()->topDocument() seems like needless indirection. You're going to the Document, which just goes back to the Frame. Why not just frame()->tree()->top()->document() ? > > I also didn't realize that you could just do this. I've been using topDocument() in all other locations, so I just did that here too. That might work better. topDocument() is fine when you're working with a Document as your context object. If you've already got a Frame, there's no reason to traverse to the Document just to traverse back to the Frame.

Created attachment 161382 [details] Patch

Comment on attachment 161382 [details] Patch Please make sure to file a bug for the followup task of dynamically updating all interested parties when the state changes, as we discussed.

Committed r127513: <http://trac.webkit.org/changeset/127513>