389632 - nsExternalHelperAppService::ExternalProtocolHandlerExists (almost) always sets aHandlerExists to PR_TRUE enabling gnomevfs protocols

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect)

Description

[Karl Tomlinson (:karlt)]

gnomevfs protocols are restricted in nsGnomeVFSProtocolHandler to disable all but trusted protocols.

However, nsOSHelperAppService::LoadUriInternal invokes gnome_url_show through nsGNOMERegistry::LoadURL.  gnome_url_show invokes gnome_vfs_url_show.

gnome_vfs_url_show uses gconf /desktop/gnome/url-handlers if a handler is found, but otherwise falls back to gnomevfs modules.  If the url is file:* or if there is an application for the mime type with %U or %u in the Exec desktop field, then that application is invoked.

nsOSHelperAppService::OSProtocolHandlerExists checks whether there is a gconf /desktop/gnome/url-handlers entry but now that the result of this check is ignored, gnome_vfs_url_show can fall back to gnomevfs.

This probably started after the commit from bug #388701.

It looks like nsOSHelperAppService::GetProtocolInfoFromOS should check exists from OSProtocolHandlerExists.

gnome-vfs issues probably won't exist on other platforms but other platforms may be affected in other ways as they also ignore check in nsOSHelperAppService::GetProtocolInfoFromOS.

Comment 1

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

when you say the result of OSProtocolHandlerExists is ignored, is that the thing that got fixed by this checkin:
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsExternalHelperAppService.cpp&branch=&root=/cvsroot&subdir=mozilla/uriloader/exthandler&command=DIFF_FRAMESET&rev1=1.319&rev2=1.320

Comment 2

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

OK, the answer is "apparently not"

Comment 3

[Karl Tomlinson (:karlt)]

(In reply to comment #1)

No.  *aHandlerInfo from GetProtocolInfoFromOS is (almost) always non-NULL.

Comment 4

[Karl Tomlinson (:karlt)]

Attachment: GetProtocolInfoFromOS only if handler exists

(os2 doesn't have GetProtocolInfoFromOS)

Comment 5

[Karl Tomlinson (:karlt)]

Attachment: comments warning about checking HandlerExists

Comment 6

[Karl Tomlinson (:karlt)]

Attachment: comments with extra word removed

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 274094 [details] [diff] [review]
comments with extra word removed

r+sr=dmose; it'd be great if you could paste the same comment into the unix version of GetProtocolInfoFromOS() too.

Comment 8

[Karl Tomlinson (:karlt)]

Attachment: comments for GetProtocolInfoFromOS

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 274100 [details] [diff] [review]
comments for GetProtocolInfoFromOS

r+sr=dmose

Comment 10

[Karl Tomlinson (:karlt)]

a=roc for M7

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: merge the first 3 patches

Comment 12

[cmtalbert]

Tested patch on windows debug build and filed bug 389866 as a follow on.

Comment 13

[Dan Mosedale (:dmosedale, :dmose)]

Merged patch checked in; thanks Karl!

Checking in exthandler/nsExternalProtocolHandler.cpp;
/cvsroot/mozilla/uriloader/exthandler/nsExternalProtocolHandler.cpp,v  <--  nsExternalProtocolHandler.cpp
new revision: 1.44; previous revision: 1.43
done
Checking in exthandler/beos/nsOSHelperAppService.cpp;
/cvsroot/mozilla/uriloader/exthandler/beos/nsOSHelperAppService.cpp,v  <--  nsOSHelperAppService.cpp
new revision: 1.23; previous revision: 1.22
done
Checking in exthandler/mac/nsOSHelperAppService.cpp;
/cvsroot/mozilla/uriloader/exthandler/mac/nsOSHelperAppService.cpp,v  <--  nsOSHelperAppService.cpp
new revision: 1.53; previous revision: 1.52
done
Checking in exthandler/unix/nsGNOMERegistry.cpp;
/cvsroot/mozilla/uriloader/exthandler/unix/nsGNOMERegistry.cpp,v  <--  nsGNOMERegistry.cpp
new revision: 1.16; previous revision: 1.15
done
Checking in exthandler/unix/nsOSHelperAppService.cpp;
/cvsroot/mozilla/uriloader/exthandler/unix/nsOSHelperAppService.cpp,v  <--  nsOSHelperAppService.cpp
new revision: 1.70; previous revision: 1.69
done
Checking in exthandler/win/nsOSHelperAppService.cpp;
/cvsroot/mozilla/uriloader/exthandler/win/nsOSHelperAppService.cpp,v  <--  nsOSHelperAppService.cpp
new revision: 1.74; previous revision: 1.73
done

Comment 14

[Daniel Veditz [:dveditz]]

I think we want this on the 1.8 branch as well, for bug 389611

Comment 15

[Karl Tomlinson (:karlt)]

Daniel,

This bug was only a regression since bug #388701 on trunk, so this shouldn't be needed on the branch.

biesi commented similarly on irc around 2007-07-26 19:19:00 PDT.

Comment 16

[Karl Tomlinson (:karlt)]

(Correcting milestone accidental change: maybe I shouldn't reload bugzilla pages.)

Comment 17

[Daniel Veditz [:dveditz]]

marking blocking minus for the branches to put the relevant information up top for the next guy who follows the dependency chain