The background thumbnail service loads pages in an off-screen browser and needs a way to prevent it from triggering downloads that appear to the user out of nowhere. This patch adds nsIDocShell.allowExternalHandling, which prevents nsURILoader from consulting the helper app service. I put the test in uriloader/base, even though there aren't any tests there, because that's close to the nsURILoader changes, which are the critical part of the patch. Justin, would you mind reviewing while bz's away? This patch is modeled on bug 759964, which added nsIDocShell.allowMedia.

All of these explicit flags make me really sad. But I guess this patch has positive karma, since it adds a test. :) I'll have a closer look on Monday.

Comment on attachment 791629 [details] [diff] [review] patch This looks good to me, but I am not totally comfortable reviewing the nsURILoader changes. They seem right to me, but I'm not sure. It seems to me that landing this patch will make things more correct than they already are. Maybe we should land it and flag bz for a post-facto review.

Hmm, I wonder if we can't actually just reuse DONT_RETARGET, rather than introducing DONT_HANDLE_EXTERNALLY? I'm not sure about the semantics, but it seems to have the same net effect (just also preventing the check for "other listeners"). Bug 309525 isn't super clear, but the other user of that flag (nsObjectLoadingContent) is in maybe a similar situation (<object> loads shouldn't trigger downloads). I don't fully understand why it wants to additionally skip the other listeners, or what those listeners are in practice, though.

> I don't fully understand why it wants to additionally skip the other listeners, or what those > listeners are in practice, though. Me either, but what you say seems pretty sensible.

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #3) > I don't fully understand why it wants to > additionally skip the other listeners, or what those listeners are in > practice, though. So, the other listeners are only relevant when the original listener (=the original docshell) refuses to load the content. Each docshell is a listener. For example, in SeaMonkey's mailnews, if you click a link in an email, the docshell displaying the email will refuse to load the http content, and the load will be retargeted to a browser window. I can't think of a circumstance where a browser window would refuse a load [of a supported mime type], so DONT_RETARGET should be identical to a hypothetical DONT_HANDLE_EXTERNALLY flag.

Comment on attachment 791629 [details] [diff] [review] patch Sounds like this should have gotten r-!

Thanks. This changes the attribute name from nsIDocShell.allowExternalHandling to allowContentRetargeting. nsURILoader::OpenChannel still sniffs the docshell but now sets the DONT_RETARGET flag. This still needs sr due to the idl change.

Comment on attachment 792459 [details] [diff] [review] nsIDocShell.allowContentRetargeting + DONT_RETARGET So, instead of changing URILoader.cpp, why not change this caller: http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#9855 to use uriloader->openChannel instead, which lets you pass the flag? I'm going to mark this review- for the moment, because realistically I won't see a response until I get another review request email... feel free to r? me again for this patch if you have a reason for not changing this.

Comment on attachment 792459 [details] [diff] [review] nsIDocShell.allowContentRetargeting + DONT_RETARGET OpenURI does more than call OpenChannel: http://mxr.mozilla.org/mozilla-central/source/uriloader/base/nsURILoader.cpp#772 Do you really want me to duplicate that in the nsDocShell caller? (Which is why I'm requesting review on the same patch.) Alternatively, I thought about adding a boolean aIsContentRetargetable parameter to nsIURILoader.openURI, or adding a second openURI method that takes flags. Would either of those options be better?

(In reply to Drew Willcoxon :adw from comment #9) > Alternatively, I thought about adding a boolean aIsContentRetargetable > parameter to nsIURILoader.openURI, or adding a second openURI method that > takes flags. Would either of those options be better? No, when it comes to that you should just change the bool argument to be a flag argument. That's probably the best solution. That won't even break existing callers in extensions, because if they pass true it will become 1 which is IS_CONTENT_PREFERRED. Should still change all callers in our code...

Sounds good. This moves the test from uriloader to docshell since uriloader doesn't have much to do with it anymore, and it updates all the openURI callers I could find.

Comment on attachment 793084 [details] [diff] [review] change nsIURILoader.openURI's parameters thanks!

Comment on attachment 793084 [details] [diff] [review] change nsIURILoader.openURI's parameters Thank you! Would you mind sr'ing too, or should I ask someone else?

Comment on attachment 793084 [details] [diff] [review] change nsIURILoader.openURI's parameters Per the policy I can't give both r and sr -- http://www.mozilla.org/hacking/reviewers.html However, you're welcome to use my review as either r or sr, whichever makes it easier for you :)

Comment on attachment 793084 [details] [diff] [review] change nsIURILoader.openURI's parameters r=me

Thanks guys! https://hg.mozilla.org/integration/mozilla-inbound/rev/e5c1679a3856 https://tbpl.mozilla.org/?tree=Try&rev=9d61eb19a7f9

Build failures like: https://tbpl.mozilla.org/php/getParsedLog.php?id=26963148&tree=Mozilla-Inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/36e5c3d1e15a

e:/builds/moz2_slave/m-in-w32-000000000000000000000/build/uriloader/base/nsURILoader.cpp(758) : error C2511: 'nsresult nsURILoader::OpenURI(nsIChannel *,uint32_t,nsIInterfaceRequestor *)' : overloaded member function not found in 'nsURILoader' wtf is this? It compiled fine on two other similar machines. If this were a legitimate compiler error, it ought to have failed on the other machines, too. (This machine is a Win 7 opt (WINNT 5.2) with slave ID w64-ix-slave150. On that same inbound push, it compiled fine on Win 7 debug w64-ix-slave148, and on the try push, on Win 7 opt w64-ix-slave26.) > Compiler Error C2511 > > No version of the function is declared with the specified parameters. http://msdn.microsoft.com/en-us/library/20fs9x81.aspx nsIURILoader.openURI is the function whose boolean parameter I changed to an unsigned long. Could this machine have gotten into a state where it's using some old output from xpidl, despite the fact that the idl changed? > e:\builds\moz2_slave\m-in-w32-000000000000000000000\build\uriloader\base\nsURILoader.h(27) : see declaration of 'nsURILoader' Is there any way I can get access to this file to see what the compiler was expecting?

Gavin mentioned bug 909035 (thanks Gavin), whose dupe bug 904743 had a follow-up fix land earlier today, so I tried again. (And then I saw that bug 908977 may be the real problem, but it's hard for me to tell what the current status of idl modifications is. Sorry if this bounces again.) https://hg.mozilla.org/integration/mozilla-inbound/rev/2ad7dd8a1201