389969 - There is no protocol handling dialog

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a7pre) Gecko/2007072722 Minefield/3.0a7pre ID:2007072722

repro:
Open FF
go to ttp://google.com (see Bug 389705)

result:
javascript Alertbox:
Firefox doesn't know how to open this address, because the protocol isn't associated with any program.

expected: the new "protocol handling dialog"

Comment 1

[Peter van der Woude [:Peter6]]

Attachment: screenshot of dialog

another case which seems to work for others
download http://www.justin.tv/widgets/jtv_player.swf
press open in the download manager

result: see screenshot (Windows Dialog)

expected: Firefox protocol handling dialog

Comment 2

[Egor Pelevin]

I need to manually register a protocol (e.g. webcal) to get the protocol handling dialog

Comment 3

[Shawn Wilsher :sdwilsh]

Does a nightly from before bug 385065 landed work the same, or is this a regression?

Comment 4

[Peter van der Woude [:Peter6]]

regressionrange:
works in 20070726_1956_firefox-3.0a7pre.en-US.win32
fails in 20070727_1135_firefox-3.0a7pre.en-US.win32

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=1185504960&maxdate=1185561299&cvsroot=%2Fcvsroot

looks like the 2007-07-27 11:31pdt checkin broke this

Comment 5

[Shawn Wilsher :sdwilsh]

OK, that is Bug 389632.

Comment 6

[Mike Connor [:mconnor]]

comment 1 mixes up protocol and content handling, I think.  Open in the download manager opens the file, there's no protocol involved at that point, and we don't have web handlers for content types hooked up yet.

comment 2 is a little confusing, since i don't remember how we'd handle a protocol that the OS didn't have a handler for, though I think it would be the same behaviour...

Comment 7

[Shawn Wilsher :sdwilsh]

However, since protocol and mime type handling are implemented on the same object, it is possible that we broke something in both (comment 1).

I'm not sure how the behavior in comment 2 is coming about...

Comment 8

[Karl Tomlinson (:karlt)]

When i submitted bug 389632, I had been looking at the code prior to Bug 385065 landing, and wasn't aware of those changes.

Now nsExternalHelperAppService::ExternalProtocolHandlerExists maybe should always return true as the user will (always?) be asked for an application.

Comment 9

[Karl Tomlinson (:karlt)]

Attachment: working hack for unix

This hack at least indicates where code changes may need to be made.

Thanks to Justin on bug 389766 for pointing out that nsMIMEInfoImpl::GetHasDefaultHandler is now what really says if there is a system specified handler.  This provides a hack fix in GetHasDefaultHandler for that bug but Justin's suggestions in bug 389766 comment 7 are better.

nsOSHelperAppService::GetProtocolInfoFromOS now should (usually) return non-null but, if the handler doesn't exist (or is disabled through gconf) it should not do anything that might make GetHasDefaultHandler set _retval to true.  (Setting the description should be fine though if Justin's suggestions in bug 389766 are carried out.)

Comment 10

[Karl Tomlinson (:karlt)]

Some of the comments added in bug 389632 should be removed also,
and we could probably remove the redundant check on HaveExternalProtocolHandler in nsExternalProtocolHandler::NewChannel.

Comment 11

[Karl Tomlinson (:karlt)]

Similar changes need to be made to other OS's too.

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #8)
> Now nsExternalHelperAppService::ExternalProtocolHandlerExists maybe should
> always return true as the user will (always?) be asked for an application.

The user will usually be asked for an application, modulo the blacklist preferences, or so my understanding of the current UI design goes.

In the short term, your suggestion might be the right one; I'm unsure.

Over the longer term, my suspicion is that one of the paths towards making this code less fragile is likely to involve dis-entraining the front-end decisions from the backend ones.  Exactly where to draw this line is likely to be the interesting question, but one could imagine the front-end making all relevant decisions about whether or not the user should be asked.

Comment 13

[Shawn Wilsher :sdwilsh]

Comment on attachment 274450 [details] [diff] [review]
working hack for unix

>-    (*aHandlerInfo)->SetPreferredAction(nsIHandlerInfo::useSystemDefault);
>+    (*aHandlerInfo)->SetPreferredAction(nsIHandlerInfo::useSystemDefault); //XXXkt what if no system default?
Check, and if we don't, set it to nsIHandlerInfo::alwaysAsk

>-  if (NS_FAILED(rv) || !exists)
>+  if (NS_FAILED(rv) /* || !exists */)
shouldn't we remove this bit instead of just commenting it out (or do some XXX comment stating why it is commented out


Otherwise, I think this is right for M7

Comment 14

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

Not so hackish way - does what dmose and I just talked about.

Comment 15

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

addresses comments

Comment 16

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 274484 [details] [diff] [review]
v1.1

sr=dmose

Comment 17

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

Comment on attachment 274484 [details] [diff] [review]
v1.1

please add a comment to the IDL saying that alwaysAsk basically means not initialized

uriloader/exthandler/nsMIMEInfoImpl.cpp
+  *_retval = !mDefaultAppDescription.IsEmpty();;

remove one of the semicolons

uriloader/exthandler/beos/nsOSHelperAppService.h
wrong indentation here

Comment 18

[Shawn Wilsher :sdwilsh]

Attachment: v1.2

for checkin

Comment 19

[Shawn Wilsher :sdwilsh]

Attachment: v1.3

typo free for checkin

Comment 20

[Shawn Wilsher :sdwilsh]

Checking in uriloader/exthandler/nsExternalHelperAppService.cpp;
new revision: 1.332; previous revision: 1.331
Checking in uriloader/exthandler/nsExternalHelperAppService.h;
new revision: 1.85; previous revision: 1.84
Checking in uriloader/exthandler/nsMIMEInfoImpl.cpp;
new revision: 1.64; previous revision: 1.63
Checking in uriloader/exthandler/beos/nsOSHelperAppService.cpp;
new revision: 1.24; previous revision: 1.23
Checking in uriloader/exthandler/beos/nsOSHelperAppService.h;
new revision: 1.14; previous revision: 1.13
Checking in uriloader/exthandler/mac/nsOSHelperAppService.cpp;
new revision: 1.54; previous revision: 1.53
Checking in uriloader/exthandler/mac/nsOSHelperAppService.h;
new revision: 1.19; previous revision: 1.18
Checking in uriloader/exthandler/unix/nsOSHelperAppService.cpp;
new revision: 1.71; previous revision: 1.70
Checking in uriloader/exthandler/unix/nsOSHelperAppService.h;
new revision: 1.24; previous revision: 1.23
Checking in uriloader/exthandler/win/nsOSHelperAppService.cpp;
new revision: 1.75; previous revision: 1.74
Checking in uriloader/exthandler/win/nsOSHelperAppService.h;
revision: 1.29; previous revision: 1.28
Checking in netwerk/mime/public/nsIMIMEInfo.idl;
new revision: 1.32; previous revision: 1.31

Comment 21

[cmtalbert]

Tested Fix on Linux (Ubuntu 7.04) and Windows XP.  On both those operating systems, the fix looks good.  Content handling falls through to the OS, as expected, and for unknown protocol handlers the protocol selection dialog is shown.

But, on Mac OS X, the protocol dialog is not shown, and the steps from Comment 0 still reproduce the bug.  Tested with: Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a7pre) Gecko/2007073016 Minefield/3.0a7pre (First hourly build to contain the fix).

Also, get this error message in the console window:
Error: Warning: unrecognized command line flag -foreground

Source File: file:///Volumes/Minefield/Minefield.app/Contents/MacOS/components/nsBrowserContentHandler.js
Line: 647

Sdwilsh believes the problem to be here:
http://mxr.mozilla.org/seamonkey/source/uriloader/exthandler/mac/nsOSHelperAppService.cpp#99

--> REOPENING bug

Comment 22

[Dan Mosedale (:dmosedale, :dmose)]

Looks like the problem is really here:

http://lxr.mozilla.org/seamonkey/source/uriloader/exthandler/mac/nsInternetConfigService.cpp#128

The HasProtocolHandler method simply returns NS_ERROR_FAILURE when it really should be returning NS_OK.  This ends up getting propagated a couple of frames up the stack and triggering the "return nsnull" here:

http://mxr.mozilla.org/seamonkey/source/uriloader/exthandler/mac/nsOSHelperAppService.cpp#326

Comment 23

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1: semi-minimal fix

Here's a semi-minimal fix for the Mac problem.  It makes nsInternetConfigService::HasProtocolHandler tell the truth about whether or not there's a handler for the given protocol, although it continues to notify its caller if the handler happens to be the current app.

Then, in nsOSHelperAppService::OSProtocolHandlerExists, instead of displaying an alert if the handler happens to be the current app, we pretend there's no handler for the given protocol.

A more robust fix would pass word of the handler loop back to the caller of OSProtocolHandlerExists, which would pass it back to its caller, and all the way up the chain until we get to the front-end code, which would either automatically resolve the problem by breaking the loop somehow or notify the user about the problem and provide the user with options for breaking the loop.

But that robust fix seems out of scope for M7 and may well be out of scope for Fx3 overall.  In any case, this semi-minimal fix resolves the current issue and makes the code a bit more transparent about what it's doing, so if we decide to implement the more robust fix later on (or just forget about it and someone excavates this code later) it'll be clearer what's going on and what needs to be done.

Comment 24

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v2: minimal fix that doesn't assume usable out param on failure return

biesi and I discussed on IRC that it would make more sense for HasProtocolHandler to leave its out param false when the handler is the current app, since it's "unusual to have usable out params in a failure return", and since we ignore the out param anyway when we see the failure at the moment.

Here's a version that does it like that.  If nsInternetConfigService::HasProtocolHandler returns NS_ERROR_NOT_AVAILABLE, then it doesn't try to set the out param to specify that there's a handler, and nsOSHelperAppService::OSProtocolHandlerExists doesn't assume anything about the value of the out param.

Comment 25

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 274570 [details] [diff] [review]
patch v2: minimal fix that doesn't assume usable out param on failure return

sr=dmose.  very nice; excising code is great!

Comment 26

[Myk Melez [:myk] [@mykmelez]]

a=shrep for followup patch per blanket approval for bugs on wiki page and bug list of M7 blockers

Checked in the followup patch:

Checking in uriloader/exthandler/mac/nsInternetConfigService.cpp;
/cvsroot/mozilla/uriloader/exthandler/mac/nsInternetConfigService.cpp,v  <--  nsInternetConfigService.cpp
new revision: 1.42; previous revision: 1.41
done
Checking in uriloader/exthandler/mac/nsOSHelperAppService.cpp;
/cvsroot/mozilla/uriloader/exthandler/mac/nsOSHelperAppService.cpp,v  <--  nsOSHelperAppService.cpp
new revision: 1.55; previous revision: 1.54
done

Comment 27

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a7pre) Gecko/2007073022 Minefield/3.0a7pre ID:2007073022

Error: (void 0) has no properties
Source file: chrome://global/content/bindings/dialog.xml
Line: 81

if the "Launch Application Dialog" is openend
(gotta dash to work so can't file a new bug)

Comment 28

[Shawn Wilsher :sdwilsh]

(In reply to comment #27)
bah!  I hit that at one point when working on this, then it went away and I couldn't reproduce it...

I'm not 100% sure what's up with it.

Comment 29

[Al Billings [:abillings - ex-MoCo]]

Verified fixed on the Mac with Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a7pre) Gecko/200707310404 Minefield/3.0a7pre.

With the steps in Comment #0, I get asked on the Mac to pick an app to handle the protocol.

Comment 30

[mcdavis941 (sporadically reading bugmail)]

related:

   "launch application dialog loses resizing state when checkbox is clicked"
   https://bugzilla.mozilla.org/show_bug.cgi?id=390555

Comment 31

[Egor Pelevin]

This is regressed again:

works in 20070809_1507_firefox-3.0a8pre.en-US.win32.zip
fails in 20070809_1524_firefox-3.0a8pre.en-US.win32.zip

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1186697220&maxdate=1186698239

bug 391380?

Comment 32

[Boris Zbarsky [:bzbarsky]]

> This is regressed again:

For what protocol?

Comment 33

[Egor Pelevin]

There is no default webcal handler on WinXP, so Firefox displays an alert "Firefox doesn't know how to open this address, because the protocol (webcal) isn't associated with any program". 

Tested on http://people.mozilla.org/~ctalbert/test-webcal-protocol.html

Comment 34

[Peter van der Woude [:Peter6]]

this is definitly broken.
Do we need a new bug ?

Comment 35

[cmtalbert]

(In reply to comment #34)
> this is definitly broken.
> Do we need a new bug ?
> 
No, This is the same issue as when it was first reported.  In this case we should reopen this bug.

--> REOPENED

Comment 36

[Shawn Wilsher :sdwilsh]

(In reply to comment #35)
> (In reply to comment #34)
> > this is definitly broken.
> > Do we need a new bug ?
> No, This is the same issue as when it was first reported.  In this case we
> should reopen this bug.
Except that something else caused this to happen again, so I think we should file a new bug.

Comment 37

[cmtalbert]

Ok, per comments here and on IRC, we have decided it would be better to open a new bug for this regression since this one is already pretty unwieldy.  The new bug is bug 392964.  I have added relevant parts of this bug and the regression information there, please add follow on comments and patches to that bug.

Resetting this bug back to RESOLVED FIXED.

Comment 38

[Marcia Knous [:marcia]]

Litmus Triage Team: ctalbert, will you handle this Litmus case?