Closed Bug 420756 Opened 13 years ago Closed 13 years ago

landing default mailto: handler fails to reset the alwaysAsk stuff

Categories

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

defect

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9beta5

People

(Reporter: dmose, Assigned: Dolske)

References

Details

(Keywords: user-doc-needed)

Attachments

(3 files, 5 obsolete files)

This was discovered by the unit tests while landing the Yahoo Mail handler in bug 413630; it was subsequently backed out as a result.
Flags: blocking1.9?
Assignee: nobody → dmose
Flags: in-testsuite+
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?
Here's an email thread that discusses how the UI is supposed to work:

Subject: injection of mailto handlers
From: Dan Mosedale <dmose@mozilla.org>
Date: Mon, 03 Mar 2008 12:30:15 -0800
To: Mike Beltzner <beltzner@mozilla.com>, Alex Faaborg <faaborg@mozilla.com>

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 <faaborg@mozilla.com>

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 <dmose@mozilla.org>

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 <beltzner@mozilla.com>

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 <dmose@mozilla.org>

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

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...
Assignee: dmose → dolske
Target Milestone: --- → mozilla1.9beta5
Version: unspecified → Trunk
Does this block all changes to localizations of mailto handlers, too?
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.
Attached patch alwaysAsk tests, p1 (obsolete) — Splinter Review
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.
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.
Attached patch Patch v.1 (obsolete) — Splinter Review
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.
Attachment #308448 - Attachment is obsolete: true
Attachment #308803 - Flags: review?(dmose)
Attached patch Patch v.2 (obsolete) — Splinter Review
Bah, sorry, had a unrelated patch for bug 299372 included in the diff.
Attachment #308803 - Attachment is obsolete: true
Attachment #308804 - Flags: review?(dmose)
Attachment #308803 - Flags: review?(dmose)
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.
Attached patch Patch v.3 (obsolete) — Splinter Review
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.
Attachment #308804 - Attachment is obsolete: true
Attachment #309024 - Flags: review?(dmose)
Attachment #308804 - Flags: review?(dmose)
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 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.
Attachment #309024 - Flags: review?(dmose)
Attached patch Patch v.4 (obsolete) — Splinter Review
Attachment #309024 - Attachment is obsolete: true
Attachment #309360 - Flags: review?(dmose)
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.
Blocks: 419213
Flags: blocking1.9? → blocking1.9+
Priority: -- → P1
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?
Attachment #309360 - Flags: review?(dmose) → review-
(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. :)

(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.
Attached patch Patch v.5Splinter Review
Fix nits.
Attachment #309360 - Attachment is obsolete: true
Attachment #310047 - Flags: review?(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);
Attachment #310047 - Flags: review?(dmose) → review+
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
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
SeaMonkey fails test_handlerService now at line 153
> *** CHECK FAILED: 1 == 0

Are we failing because we don't have bug 413630?
"we" in this case being 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
Attachment #310235 - Attachment is patch: true
Attachment #310235 - Attachment mime type: application/octet-stream → text/plain
Attached patch Same but -wSplinter Review
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].
(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. :-)]
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.