420756 - landing default mailto: handler fails to reset the alwaysAsk stuff

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect, P1)

Description

[Dan Mosedale (:dmosedale, :dmose)]

This was discovered by the unit tests while landing the Yahoo Mail handler in bug 413630; it was subsequently backed out as a result.

Comment 1

[(not currently active) Ted Mielczarek]

Could you elaborate on what's broken? Is this a real bug that was exposed by changing the prefs, or just a bad assumption in the tests?

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

Here's an email thread that discusses how the UI is supposed to work:

Subject: injection of mailto handlers
From: Dan Mosedale <[e-mail]>
Date: Mon, 03 Mar 2008 12:30:15 -0800
To: Mike Beltzner <[e-mail]>, Alex Faaborg <[e-mail]>

I know we had a bunch of discussion about this at one point, and I'm not 100% positive where we ended up.  So far, I haven't been able to find the bug where this discussed, so I'm hoping one of you guys remembers the answer:

In an existing profile, if somebody has their default set to never ask on a protocol, and Firefox injects a new handler (eg the Yahoo mailto handler), should it also reset the protocol to always ask, so that people are made aware of the new option for handling mail?  Or should it leave things alone, and they'll only be aware of it if someone tells them or they run across it while exploring the prefs?

Dan


From: Alex Faaborg <[e-mail]>

It feels like leaving things alone will be less annoying, if the user is clearly using a client side app, I'm not sure there is a very high probability that they will want to switch to an online app.

Perhaps for Firefox 4 we can inspect the user's history to set defaults more intelligently.

-Alex


From: Dan Mosedale <[e-mail]>

In the current code base, in the mailto case, we can't really know whether or not the user has ever used (successfully or not) a mailto: link.  Does that make any difference?

Dan


From: Mike Beltzner <[e-mail]>

Dan Mosedale wrote:
> In an existing profile, if somebody has their default set to never ask on a 
> protocol, and Firefox injects a new handler (eg the Yahoo mailto 

How would Firefox inject this new handler?

If it's a matter of profile migration or whatnot, we should absolutely continue to honour the user's preference to never ask.

If it's a matter of adding a handler due to some user-confirmed action (like going to Yahoo! and clicking "add me as a mail handler") then we should reset the user's preference and ask.

cheers,
mike


From: Dan Mosedale <[e-mail]>

Mike Beltzner wrote:
> Dan Mosedale wrote:
>> In an existing profile, if somebody has their default set to never ask on
>> a protocol, and Firefox injects a new handler (eg the Yahoo mailto 
>
> How would Firefox inject this new handler?
>
> If it's a matter of profile migration or whatnot, we should absolutely 
> continue to honour the user's preference to never ask.

In the case of mailto, by default, we never ask; it's not something that user has to explicitly set.

Dan

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

There are definitely some issues with the tests, and I thought there was an actual bug as well, but I'm now less sure.  More as I figure it out...

Comment 4

[Axel Hecht [:Pike]]

Does this block all changes to localizations of mailto handlers, too?

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

Well, until this gets fixed, checking in a mailto: handler in any locale would cause the same problem for builds of that locale.  Part of the reason we were so aggressive backing out was because this problem was detected immediately prior to tagging.

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: alwaysAsk tests, p1

When I was poking at this around the time of the backout, I figured out that part of the deal was that we needed some more tests.  Here's a patch that adds these tests, and I _think_ they're checking for the proper values.  For reasons I didn't get to the point of digging into, they don't all pass.

Comment 7

[Justin Dolske [:Dolske]]

So, what was the breakage that caused the backout? It's still not really clear here.

I agree with what Beltzner wrote in your pasted thread...

* If the user adds a new handler via a notification bar, we should reset alwaysAsk.
* If *we* add (ship) a new handler, the user's preference should not be changed.

Comment 8

[Justin Dolske [:Dolske]]

Attachment: Patch v.1

So, this seems to fix the immediate problem. After much poking and prodding in the code, it looks like the problem was that the pref (which the test sets) wasn't ever being checked if there was a protocol handler installed... If nsExternalHelperAppService::GetProtocolHandlerInfo() made a successful call to handlerSvc->FillHandlerInfo(), it never called SetProtocolHandlerDefaults() which is where the pref was checked.

I think this should still probably get some manual testing to confirm that we're doing the right thing in the Real World.

Comment 9

[Justin Dolske [:Dolske]]

Attachment: Patch v.2

Bah, sorry, had a unrelated patch for bug 299372 included in the diff.

Comment 10

[Justin Dolske [:Dolske]]

Ugh. So, there's a sucky problem here.

We ship a default pref network.protocol-handler.warn-external.mailto set to false. With this patch as-is, that pref now overrides the value stored in the handler service (ie, mimetypes.rdf)... Which means if you go into Preferences -> Applications, and set "mailto" "Always Ask", we'll actually never ask, and if you close/reopen the pref window, your choice appears to have reverted back.

So, not sure what to do here. We could use pref.hasUserValue(), but that's false if the pref value is set to the same as the default. We could ignore the pref unless it's |true|, but then the tests might have problems with that?

God, I HATE this code.

Comment 11

[Justin Dolske [:Dolske]]

Attachment: Patch v.3

This resolves the pref issue in two ways:

* Only use the pref value when it's set to true.
* Keep the pref value in sync with the value stored in mimeTypes.rdf.

Comment 12

[Justin Dolske [:Dolske]]

Oh, I also manually tested that we're not breaking anything by:

1) Create a new profile with a current nightly [which has no default mailto handlers]
2) Check that clicking a mailto: opens a Thunderbird compose window without prompting
3) Quit browser, start my FF build (which has this patch and the new default handlers from bug 413630)
4) Check that clicking a mailto: still works as in #2
5) Change Prefs -> Applications to "Always ask"
6) Confirm pref got updated
7) Check that clicking mailto: prompts for an action
8) Change Prefs -> Applications to "Yahoo"
9) Check that clicking mailto: opens Yahoo without prompting.

The tests should exercise all this too, but wanted to be sure.

Comment 13

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 309024 [details] [diff] [review]
Patch v.3

dolske and I just had a f2f powwow about this and there's now a theory for a new and better patch.

Comment 14

[Justin Dolske [:Dolske]]

Attachment: Patch v.4

Comment 15

[Justin Dolske [:Dolske]]

Err, forgot to comment:

This no longer tries to keep the pref value in sync with the RDF value. The pref is now used as the initial default, and the RDF value will always be preferred.

Comment 16

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 309360 [details] [diff] [review]
Patch v.4

The fix looks good.   I have some questions about the tests, though...

>-  // OS default exists, explicit warning pref: false
>+  // OS default exists, injected default does not exist, 
>+  // explicit warning pref: false
>   const kExternalWarningPrefPrefix = "network.protocol-handler.warn-external.";
>-  prefSvc.setBoolPref(kExternalWarningPrefPrefix + "mailto", false)
>-  protoInfo = protoSvc.getProtocolHandlerInfo("mailto");
>+  prefSvc.setBoolPref(kExternalWarningPrefPrefix + "http", false);
>+  protoInfo = protoSvc.getProtocolHandlerInfo("http");
>+  do_check_eq(0, protoInfo.possibleApplicationHandlers.length);

I'm confused about why the above test would (or should) pass.  Every OS should have at least one handler for http: the default browser.

>+  // OS default exists, injected default exists, explicit warning pref: false
>+  prefSvc.setBoolPref(kExternalWarningPrefPrefix + "mailto", false);
>+  protoInfo = protoSvc.getProtocolHandlerInfo("mailto");
>+  do_check_eq(1, protoInfo.possibleApplicationHandlers.length);

Similarly, with an injected default and an OS default, I would expect this to be 2, not 1.  The other tests of possibleApplicationHandlers.length have the same issue, so I'm not going to keep calling it out.  If these are actually passing, maybe this has detected a bug in the underlying code?  I'm basing this on the description of possibleApplicationHandlers in nsIMIMEInfo.idl.

>+  // OS default exists, injected default exists, explicit warning pref: true
>+  prefSvc.setBoolPref(kExternalWarningPrefPrefix + "mailto", true);
>   protoInfo = protoSvc.getProtocolHandlerInfo("mailto");
>+  dump("preferredAction = " + protoInfo.preferredAction + "\n");

The dump() is cruft left over from my first iteration; I'd say get rid of it.

>+  do_check_eq(1, protoInfo.possibleApplicationHandlers.length);
>+  // This is false, because the injected mailto handler carried over the
>+  // default pref value, and so when we set the pref above to true it's ignored.
>+  do_check_false(protoInfo.alwaysAskBeforeHandling);
>+  // Set the value stored in RDF to true, make sure we get the right value.
>+  prefSvc.setBoolPref(kExternalWarningPrefPrefix + "mailto", false);
>+  protoInfo.alwaysAskBeforeHandling = true;
>+  handlerSvc.store(protoInfo);
>+  protoInfo = protoSvc.getProtocolHandlerInfo("mailto");
>+  do_check_eq(1, protoInfo.possibleApplicationHandlers.length);
>   do_check_true(protoInfo.alwaysAskBeforeHandling);

I'm having a hard time wrapping my head around exactly what's being tested here in this whole last clause.  Can you modify the comments to provide more clarity?

Comment 17

[Justin Dolske [:Dolske]]

(In reply to comment #16)

> >+  protoInfo = protoSvc.getProtocolHandlerInfo("http");
> >+  do_check_eq(0, protoInfo.possibleApplicationHandlers.length);
> 
> I'm confused about why the above test would (or should) pass.  Every OS should
> have at least one handler for http: the default browser.

This is the stupid way OS handlers don't get listed in possibleApplicationHandlers (iirc they only get listed in preferredHandler or something?). This shouldn't have been affected by this checkin, I just thought it would be a good idea to check the length to ensure we don't have more/less handlers than expected.

> >+  do_check_eq(1, protoInfo.possibleApplicationHandlers.length);
> >+  // This is false, because the injected mailto handler carried over the
> >+  // default pref value, and so when we set the pref above to true it's ignored.
> >+  do_check_false(protoInfo.alwaysAskBeforeHandling);
> >+  // Set the value stored in RDF to true, make sure we get the right value.
> >+  prefSvc.setBoolPref(kExternalWarningPrefPrefix + "mailto", false);
> >+  protoInfo.alwaysAskBeforeHandling = true;
> >+  handlerSvc.store(protoInfo);
> >+  protoInfo = protoSvc.getProtocolHandlerInfo("mailto");
> >+  do_check_eq(1, protoInfo.possibleApplicationHandlers.length);
> >   do_check_true(protoInfo.alwaysAskBeforeHandling);
> 
> I'm having a hard time wrapping my head around exactly what's being tested here
> in this whole last clause.  Can you modify the comments to provide more
> clarity?

Sure. The line just above this was:

prefSvc.setBoolPref(kExternalWarningPrefPrefix + "mailto", true);

And we're testing to see that the alwaysAskBeforeHandling is false (ie, that the pref is ignored because we're using the value in RDF instead, which is false). We then test the reverse condition (pref=false, rdf=true). That's partially a holdover from the earlier patch revision, but a test is a test. :)

Comment 18

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #17)
> (In reply to comment #16)
> 
> > >+  protoInfo = protoSvc.getProtocolHandlerInfo("http");
> > >+  do_check_eq(0, protoInfo.possibleApplicationHandlers.length);
> > 
> > I'm confused about why the above test would (or should) pass.  Every OS should
> > have at least one handler for http: the default browser.
> 
> This is the stupid way OS handlers don't get listed in
> possibleApplicationHandlers (iirc they only get listed in preferredHandler or
> something?). This shouldn't have been affected by this checkin, I just thought
> it would be a good idea to check the length to ensure we don't have more/less
> handlers than expected.

Oh right.  Sigh.  How about least adding a comment to the tests pointing that once default handlers become impls of nsILocalHandlerApp, these numbers in the tests will need to change.

> > I'm having a hard time wrapping my head around exactly what's being tested here
> > in this whole last clause.  Can you modify the comments to provide more
> > clarity?
> 
> Sure. The line just above this was:
> 

I actually meant to request that you modify the comments in the patch itself.  That way the next person that reads that code won't need to come back to this bug to understand it.

Comment 19

[Justin Dolske [:Dolske]]

Attachment: Patch v.5

Fix nits.

Comment 20

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 310047 [details] [diff] [review]
Patch v.5

>Index: uriloader/exthandler/nsHandlerService.js
>===================================================================
>RCS file: /cvsroot/mozilla/uriloader/exthandler/nsHandlerService.js,v
>retrieving revision 1.20
>diff -u -p -8 -r1.20 nsHandlerService.js
>--- uriloader/exthandler/nsHandlerService.js	20 Feb 2008 04:46:35 -0000	1.20
>+++ uriloader/exthandler/nsHandlerService.js	17 Mar 2008 20:01:38 -0000
>@@ -350,24 +350,38 @@ HandlerService.prototype = {
>     aHandlerInfo.preferredApplicationHandler =
>       this._retrieveHandlerApp(preferredHandlerID);
> 
>     // Fill the array of possible handlers with the ones in the datastore.
>     this._fillPossibleHandlers(infoID,
>                                aHandlerInfo.possibleApplicationHandlers,
>                                aHandlerInfo.preferredApplicationHandler);
> 
>-    // Retrieve the "always ask" flag.
>-    // Note: we only set the flag to false if we are absolutely sure the user
>-    // does not want to be asked.  Any sort of bogus data should mean we ask.
>-    // So there must be an "alwaysAsk" property in the datastore for the handler
>-    // info object, and it must be set to "false", in order for us not to ask.
>-    aHandlerInfo.alwaysAskBeforeHandling =
>-      !this._hasValue(infoID, NC_ALWAYS_ASK) ||
>-      this._getValue(infoID, NC_ALWAYS_ASK) != "false";
>+    // If we have an "always ask" flag stored in the RDF, always use its
>+    // value. Otherwise, use the default value stored in the pref service.
>+    var alwaysAsk;
>+    if (this._hasValue(infoID, NC_ALWAYS_ASK)) {
>+      alwaysAsk = (this._getValue(infoID, NC_ALWAYS_ASK) != "false");
>+    } else {
>+      var prefSvc = Cc["@mozilla.org/preferences-service;1"].
>+                    getService(Ci.nsIPrefService);
>+      var prefBranch = prefSvc.getBranch("network.protocol-handler.");
>+      try {
>+        alwaysAsk = prefBranch.getBoolPref("warn-external." + type);
>+      } catch (e) {
>+        // will throw if pref didn't exist.
>+        try {
>+          alwaysAsk = prefBranch.getBoolPref("warn-external-default");
>+        } catch (e) {
>+          // Nothing to tell us what to do, so be paranoid and prompt.
>+          alwaysAsk = true;
>+        }
>+      }
>+    }
>+    aHandlerInfo.alwaysAskBeforeHandling = alwaysAsk;
> 
>     // If the object represents a MIME type handler, then also retrieve
>     // any file extensions.
>     if (aHandlerInfo instanceof Ci.nsIMIMEInfo)
>       for each (let fileExtension in this._retrieveFileExtensions(typeID))
>         aHandlerInfo.appendExtension(fileExtension);
>   },
> 
>Index: uriloader/exthandler/tests/unit/test_handlerService.js
>===================================================================
>RCS file: /cvsroot/mozilla/uriloader/exthandler/tests/unit/test_handlerService.js,v
>retrieving revision 1.15
>diff -u -p -8 -r1.15 test_handlerService.js
>--- uriloader/exthandler/tests/unit/test_handlerService.js	3 Dec 2007 21:45:02 -0000	1.15
>+++ uriloader/exthandler/tests/unit/test_handlerService.js	17 Mar 2008 20:01:38 -0000
>@@ -124,27 +124,60 @@ function run_test() {
> 
>   // XXX add more thorough protocol info property checking
>   
>   // no OS default handler exists
>   var protoInfo = protoSvc.getProtocolHandlerInfo("x-moz-rheet");
>   do_check_eq(protoInfo.preferredAction, protoInfo.alwaysAsk);
>   do_check_true(protoInfo.alwaysAskBeforeHandling);
>   
>-  // OS default exists, explicit warning pref: false
>+  // OS default exists, injected default does not exist, 
>+  // explicit warning pref: false
>   const kExternalWarningPrefPrefix = "network.protocol-handler.warn-external.";
>-  prefSvc.setBoolPref(kExternalWarningPrefPrefix + "mailto", false)
>-  protoInfo = protoSvc.getProtocolHandlerInfo("mailto");
>+  prefSvc.setBoolPref(kExternalWarningPrefPrefix + "http", false);
>+  protoInfo = protoSvc.getProtocolHandlerInfo("http");
>+  do_check_eq(0, protoInfo.possibleApplicationHandlers.length);
>   do_check_false(protoInfo.alwaysAskBeforeHandling);
>   
>-  // OS default exists, explicit warning pref: true
>-  prefSvc.setBoolPref(kExternalWarningPrefPrefix + "mailto", true)
>+  // OS default exists, injected default does not exist, 
>+  // explicit warning pref: true
>+  prefSvc.setBoolPref(kExternalWarningPrefPrefix + "http", true);
>+  protoInfo = protoSvc.getProtocolHandlerInfo("http");
>+  // OS handler isn't included in possibleApplicationHandlers, so length is 0
>+  // Once they become instances of nsILocalHandlerApp, this number will need
>+  // to change.
>+  do_check_eq(0, protoInfo.possibleApplicationHandlers.length);
>+  do_check_true(protoInfo.alwaysAskBeforeHandling);
>+
>+  // OS default exists, injected default exists, explicit warning pref: false
>+  prefSvc.setBoolPref(kExternalWarningPrefPrefix + "mailto", false);
>+  protoInfo = protoSvc.getProtocolHandlerInfo("mailto");
>+  do_check_eq(1, protoInfo.possibleApplicationHandlers.length);
>+  do_check_false(protoInfo.alwaysAskBeforeHandling);
>+
>+  // OS default exists, injected default exists, explicit warning pref: true
>+  prefSvc.setBoolPref(kExternalWarningPrefPrefix + "mailto", true);
>   protoInfo = protoSvc.getProtocolHandlerInfo("mailto");
>+  do_check_eq(1, protoInfo.possibleApplicationHandlers.length);
>+  // alwaysAskBeforeHandling is expected to be false here, because although
>+  // the pref is true, the value in RDF is false. The injected mailto handler
>+  // carried over the default pref value, and so when we set the pref above to
>+  // true it's ignored.
>+  do_check_false(protoInfo.alwaysAskBeforeHandling);
>+  // Now set the value stored in RDF to true, and the pref to false, to make
>+  // sure we still get the right value. (Basically, same thing as above but
>+  // with the values reversed.)
>+  prefSvc.setBoolPref(kExternalWarningPrefPrefix + "mailto", false);
>+  protoInfo.alwaysAskBeforeHandling = true;
>+  handlerSvc.store(protoInfo);
>+  protoInfo = protoSvc.getProtocolHandlerInfo("mailto");
>+  do_check_eq(1, protoInfo.possibleApplicationHandlers.length);
>   do_check_true(protoInfo.alwaysAskBeforeHandling);
>-  
>+
>+
>   //**************************************************************************//
>   // Test Round-Trip Data Integrity
> 
>   // Test round-trip data integrity by setting the properties of the handler
>   // info object to different values, telling the handler service to store the
>   // object, and then retrieving a new info object for the same type and making
>   // sure its properties are identical.
> 
>@@ -169,22 +202,23 @@ function run_test() {
>   do_check_false(handlerInfo.alwaysAskBeforeHandling);
> 
>   // Make sure the handler service's enumerate method lists all known handlers.
>   var handlerInfo2 = mimeSvc.getFromTypeAndExtension("nonexistent/type2", null);
>   handlerSvc.store(handlerInfo2);
>   var handlerTypes = ["nonexistent/type", "nonexistent/type2"];
>   try { 
>     // If we have a defaultHandlersVersion pref, then assume that we're in the
>-    // firefox tree and that we'll also have an added webcal handler.
>+    // firefox tree and that we'll also have default handlers.
>     // Bug 395131 has been filed to make this test work more generically
>     // by providing our own prefs for this test rather than this icky
>     // special casing.
>     rootPrefBranch.getCharPref("gecko.handlerService.defaultHandlersVersion");
>     handlerTypes.push("webcal");
>+    handlerTypes.push("mailto");
>   } catch (ex) {}   
>   var handlers = handlerSvc.enumerate();
>   while (handlers.hasMoreElements()) {
>     var handler = handlers.getNext().QueryInterface(Ci.nsIHandlerInfo);
>     do_check_neq(handlerTypes.indexOf(handler.type), -1);
>     handlerTypes.splice(handlerTypes.indexOf(handler.type), 1);
>   }
>   do_check_eq(handlerTypes.length, 0);

Comment 21

[Justin Dolske [:Dolske]]

Checking in uriloader/exthandler/nsHandlerService.js;
  new revision: 1.21; previous revision: 1.20
Checking in uriloader/exthandler/tests/unit/test_handlerService.js;
  new revision: 1.16; previous revision: 1.15

Comment 22

[Andrew Schultz]

SeaMonkey fails test_handlerService now at line 153
> *** CHECK FAILED: 1 == 0

Are we failing because we don't have bug 413630?

Comment 23

[jag (Peter Annema)]

"we" in this case being SeaMonkey.

Comment 24

[jag (Peter Annema)]

Attachment: Checked this in to fix this unit test failing for SeaMonkey.

Checking in uriloader/exthandler/tests/unit/test_handlerService.js;
/cvsroot/mozilla/uriloader/exthandler/tests/unit/test_handlerService.js,v  <--  test_handlerService.js
new revision: 1.17; previous revision: 1.16
done

Comment 25

[jag (Peter Annema)]

Attachment: Same but -w

I think this is the right fix (it passed make check for both SeaMonkey and Firefox), but couldn't get a hold of dolske, so this is my attempt to avoid backing out attachment 310047 [details] [diff] [review] and attachment 306643 [details] [diff] [review].

Comment 26

[Justin Dolske [:Dolske]]

(In reply to comment #25)

> I think this is the right fix (it passed make check for both SeaMonkey and
> Firefox)

Yeah, I think this is fine. [Or at least no more evil than the existing hack it's extending. :-)]