There is no protocol handling dialog

RESOLVED FIXED in Firefox 3 alpha7

Status

()

RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Peter6, Assigned: sdwilsh)

Tracking

(Blocks: 1 bug, {regression})

Trunk
Firefox 3 alpha7
x86
All
regression
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3 +
in-testsuite -
in-litmus ?

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 5 obsolete attachments)

(Reporter)

Description

11 years ago
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"
(Reporter)

Comment 1

11 years ago
Created attachment 274303 [details]
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

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

Comment 3

11 years ago
Does a nightly from before bug 385065 landed work the same, or is this a regression?
(Reporter)

Comment 4

11 years ago
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
Keywords: regression
(Assignee)

Comment 5

11 years ago
OK, that is Bug 389632.
Blocks: 389632
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...
(Assignee)

Comment 7

11 years ago
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...
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.
Created attachment 274450 [details] [diff] [review]
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.)
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.
Similar changes need to be made to other OS's too.

Updated

11 years ago
OS: Windows XP → All

Updated

11 years ago
Attachment #274450 - Attachment description: working hack → working hack for unix
(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.
(Assignee)

Comment 13

11 years ago
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

Updated

11 years ago
Flags: blocking-firefox3+
Target Milestone: --- → Firefox 3 M7
(Assignee)

Comment 14

11 years ago
Created attachment 274480 [details] [diff] [review]
v1.0

Not so hackish way - does what dmose and I just talked about.
Assignee: nobody → sdwilsh
Attachment #274450 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #274480 - Flags: superreview?(dmose)
(Assignee)

Comment 15

11 years ago
Created attachment 274484 [details] [diff] [review]
v1.1

addresses comments
Attachment #274480 - Attachment is obsolete: true
Attachment #274484 - Flags: superreview?(dmose)
Attachment #274480 - Flags: superreview?(dmose)
Comment on attachment 274484 [details] [diff] [review]
v1.1

sr=dmose
Attachment #274484 - Flags: superreview?(dmose) → superreview+
(Assignee)

Updated

11 years ago
Attachment #274484 - Flags: review?(cbiesinger)
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
Attachment #274484 - Flags: review?(cbiesinger) → review+
(Assignee)

Comment 18

11 years ago
Created attachment 274516 [details] [diff] [review]
v1.2

for checkin
Attachment #274484 - Attachment is obsolete: true
(Assignee)

Comment 19

11 years ago
Created attachment 274539 [details] [diff] [review]
v1.3

typo free for checkin
Attachment #274516 - Attachment is obsolete: true
(Assignee)

Comment 20

11 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Flags: in-testsuite-
Flags: in-litmus?
Resolution: --- → FIXED

Updated

11 years ago
Blocks: 389866

Updated

11 years ago
Blocks: 389766

Comment 21

11 years ago
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Updated

11 years ago
Whiteboard: [need new patch]
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
Created attachment 274567 [details] [diff] [review]
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.
Attachment #274567 - Flags: superreview?(dmose)
Attachment #274567 - Flags: review?(cbiesinger)

Updated

11 years ago
Whiteboard: [need new patch] → [need review biesi, sr dmose]
Attachment #274567 - Flags: review?(cbiesinger) → review+
Created attachment 274570 [details] [diff] [review]
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.
Attachment #274567 - Attachment is obsolete: true
Attachment #274570 - Flags: superreview?(dmose)
Attachment #274570 - Flags: review?(cbiesinger)
Attachment #274567 - Flags: superreview?(dmose)
Attachment #274570 - Flags: review?(cbiesinger) → review+
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!
Attachment #274570 - Flags: superreview?(dmose) → superreview+
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
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED
Whiteboard: [need review biesi, sr dmose]
(Reporter)

Comment 27

11 years ago
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)
(Assignee)

Comment 28

11 years ago
(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.
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.

Updated

11 years ago
Depends on: 390397
related:

   "launch application dialog loses resizing state when checkbox is clicked"
   https://bugzilla.mozilla.org/show_bug.cgi?id=390555
Blocks: 390397
No longer depends on: 390397
(Assignee)

Updated

11 years ago
Depends on: 391380

Comment 31

11 years ago
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?
> This is regressed again:

For what protocol?

Comment 33

11 years ago
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
(Reporter)

Comment 34

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

Comment 35

11 years ago
(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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 36

11 years ago
(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

11 years ago
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.
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED
Litmus Triage Team: ctalbert, will you handle this Litmus case?
You need to log in before you can comment on or make changes to this bug.