906276 - Add attribute to nsIDocShell to disable external handling with the helper app service

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Drew Willcoxon :adw]

Attachment: patch

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.

Comment 1

[Justin Lebar (not reading bugmail)]

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 2

[Justin Lebar (not reading bugmail)]

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.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 4

[Justin Lebar (not reading bugmail)]

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

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to :Gavin Sharp (use [e-mail] 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 6

[Justin Lebar (not reading bugmail)]

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

Sounds like this should have gotten r-!

Comment 7

[Drew Willcoxon :adw]

Attachment: nsIDocShell.allowContentRetargeting + DONT_RETARGET

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 8

[Christian :Biesinger (don't email me, ping me on IRC)]

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 9

[Drew Willcoxon :adw]

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?

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 11

[Drew Willcoxon :adw]

Attachment: change nsIURILoader.openURI's parameters

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 12

[Christian :Biesinger (don't email me, ping me on IRC)]

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

thanks!

Comment 13

[Drew Willcoxon :adw]

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 14

[Christian :Biesinger (don't email me, ping me on IRC)]

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 15

[Justin Lebar (not reading bugmail)]

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

r=me

Comment 16

[Drew Willcoxon :adw]

Thanks guys!

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

Comment 17

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Build failures like:

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

Comment 18

[Drew Willcoxon :adw]

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?

Comment 19

[Drew Willcoxon :adw]

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

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2ad7dd8a1201