1063529 - [10.10] Default Browser Notification Bar and the Preferences callers into the shellservice should try...catch their calls so things don't break when the shell service throws an exception

Status: VERIFIED FIXED

(Firefox :: Shell Integration, defect)

Description

[Sevaan Franks [:sevaan]]

I was just testing our new default browser notification bar prompt on Nightly, and after clicking "Use Nightly as my default browser" it doesn't dismiss.

STR: Opened another browser and made it my default. Opened Nightly to receive the prompt.

http://cl.ly/image/1N2k1D3d1x0w

Comment 1

[:Gijs (he/him)]

Any errors in the browser console? Does the prompt reappear when you restart?

Comment 2

[:Gijs (he/him)]

[Tracking Requested - why for this release]:
This was shipped in 34, I don't know that we changed it at any point since - Sevaan, can you reproduce on aurora (34) as well?

Comment 3

[Sevaan Franks [:sevaan]]

(In reply to :Gijs Kruitbosch from comment #1)
> Any errors in the browser console? Does the prompt reappear when you restart?

No errors in the console, and no, the bar doesn't reappear after, so it is registering my choice.

Comment 4

[Sevaan Franks [:sevaan]]

(In reply to :Gijs Kruitbosch from comment #2)
> [Tracking Requested - why for this release]:
> This was shipped in 34, I don't know that we changed it at any point since -
> Sevaan, can you reproduce on aurora (34) as well?

Has the new Aurora update been pushed? I just ran all the updates and it says it's up-to-date at v33.0a2 (2014-09-02)

Comment 5

[:Gijs (he/him)]

(In reply to Sevaan Franks [:sevaan] from comment #0)
> I was just testing our new default browser notification bar prompt on
> Nightly, and after clicking "Use Nightly as my default browser" it doesn't
> dismiss.
> 
> STR: Opened another browser and made it my default. Opened Nightly to
> receive the prompt.
> 
> http://cl.ly/image/1N2k1D3d1x0w

FWIW, I can't reproduce this by creating a new profile on today's Nightly...

Comment 6

[:Gijs (he/him)]

(In reply to Sevaan Franks [:sevaan] from comment #4)
> (In reply to :Gijs Kruitbosch from comment #2)
> > [Tracking Requested - why for this release]:
> > This was shipped in 34, I don't know that we changed it at any point since -
> > Sevaan, can you reproduce on aurora (34) as well?
> 
> Has the new Aurora update been pushed? I just ran all the updates and it
> says it's up-to-date at v33.0a2 (2014-09-02)

There should be one by now, if not, download one from https://aurora.mozilla.org/

Comment 7

[Sevaan Franks [:sevaan]]

(In reply to :Gijs Kruitbosch from comment #6)
> There should be one by now, if not, download one from
> https://aurora.mozilla.org/

The bar does not dismiss in Aurora...and it's still remaining in the new Nightly as well for me. I tested both with my old profile and with a brand new profile.

Comment 8

[:Gijs (he/him)]

(In reply to Sevaan Franks [:sevaan] from comment #7)
> (In reply to :Gijs Kruitbosch from comment #6)
> > There should be one by now, if not, download one from
> > https://aurora.mozilla.org/
> 
> The bar does not dismiss in Aurora...and it's still remaining in the new
> Nightly as well for me. I tested both with my old profile and with a brand
> new profile.

Where are your Nightly/Aurora instances on disk (mine's in /Application/FirefoxNightly.app/ ) and what other browser are you selecting when Nightly isn't the default?

Comment 9

[Sevaan Franks [:sevaan]]

> Where are your Nightly/Aurora instances on disk (mine's in
> /Application/FirefoxNightly.app/ ) and what other browser are you selecting
> when Nightly isn't the default?

/Applications/FirefoxNightly.app and /Applications/FirefoxAurora.app

I've switched to Chrome, and I've switched to other versions of Firefox. No difference in the experience for me.

Comment 10

[:Gijs (he/him)]

This seems to be yosemite-specific, and the browser console shows:

NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIShellService.setDefaultBrowser]

Looks like the OS X shellservice is unhappy about assigning the default on 10.10? Potentially has something to do with the dialog that OS X shows you ("Do you want to change your default web browser to “Nightly” or keep using “Safari”?" etc.)

Comment 11

[Steven Michaud [:smichaud] (Retired)]

Here's what I see on Yosemite DP6 and DP7, testing with the 2014-08-13 mozilla-central nightly and later.  That nightly was the first to contain the patch for bug 951627.

1) Make some other browser the default (Safari, a Firefox release, or whatever).
2) Run FirefoxNightly and see the default browser notification bar.
   Click on the "Use Nightly as my default browser" button.
3) Notice that the OS (specifically the System control panel) displays the following dialog:

   Do you want to change your default web browser to "Nightly" or keep using "[whatever]"?
   [Keep whatever]  [Use Nightly]

4) Click on the "Use Nightly" button.
   Observe that the notification bar doesn't get dismissed.

I strongly suspect this isn't a Cocoa widgets bug.  Rather, I expect it's a bug in the CSS/JavaScript code that displays the notification bar.  That code probably fails to self-dismiss whenever another dialog gets displayed as a result of doing something that would normally cause self-dismissal.

I don't see any console errors.

This bug doesn't happen on versions of OS X (10.9.X and earlier) where no dialog gets displayed as a consequence of clicking on the "Use Nightly" button.

Does anyone here know where the code lives that displays the notification bar?

Comment 12

[:Gijs (he/him)]

(In reply to Steven Michaud from comment #11)
> Here's what I see on Yosemite DP6 and DP7, testing with the 2014-08-13
> mozilla-central nightly and later.  That nightly was the first to contain
> the patch for bug 951627.
> 
> 1) Make some other browser the default (Safari, a Firefox release, or
> whatever).
> 2) Run FirefoxNightly and see the default browser notification bar.
>    Click on the "Use Nightly as my default browser" button.
> 3) Notice that the OS (specifically the System control panel) displays the
> following dialog:
> 
>    Do you want to change your default web browser to "Nightly" or keep using
> "[whatever]"?
>    [Keep whatever]  [Use Nightly]

Right.

> 4) Click on the "Use Nightly" button.
>    Observe that the notification bar doesn't get dismissed.
> 
> I strongly suspect this isn't a Cocoa widgets bug.  Rather, I expect it's a
> bug in the CSS/JavaScript code that displays the notification bar.  That
> code probably fails to self-dismiss whenever another dialog gets displayed
> as a result of doing something that would normally cause self-dismissal.

No, this is all one method that explicitly sets a default and closes the prompt. The only reasonable reason for that not happening is the setting of the default triggering an exception (such as the one cited below). We trigger the shell service's setAsDefault method, and then call this.closePrompt().


> I don't see any console errors.

That's very strange, they clearly show up for me in the browser console. I don't know why they wouldn't show up for you.

> Does anyone here know where the code lives that displays the notification
> bar?

https://hg.mozilla.org/mozilla-central/rev/72de0ffdfbd6#l1.133

Comment 13

[:Gijs (he/him)]

Also note that the other dialog is actually not shown by our widget code, but by OS X - you can easily tell on 10.10 because the default button has the correct text color - none of our dialogs do (separate bug).

Comment 14

[Steven Michaud [:smichaud] (Retired)]

Note that I'm testing with "regular" mozilla-central nightlies -- not debug nightlies.

Also, I'm testing on (two different) clean installs -- one of DP6 and the other of DP7.

Try testing with a clean profile.

And yes, I already noticed that the "Do you want to change your default" dialog is displayed by the OS.  See my step 3 in comment #11.

Comment 15

[Steven Michaud [:smichaud] (Retired)]

Oops, I was looking in the wrong "console" -- the system one.

I do see the nsIShellService.setDefaultBrowser component returned failure code in the "browser console".  It happens before the OS dialog is even dismissed.

I still think this isn't a Cocoa widgets bug.  But I'll probably have to track down the nsIShellService.setDefaultBrowser error to prove it.  Since I don't think this is a particularly urgent bug, that will take me a while.

Comment 16

[:Gijs (he/him)]

(In reply to Steven Michaud from comment #15)
> I still think this isn't a Cocoa widgets bug.  But I'll probably have to
> track down the nsIShellService.setDefaultBrowser error to prove it.  Since I
> don't think this is a particularly urgent bug, that will take me a while.

I actually only just realized that the shellservice wasn't part of widget. :-\

Moving this there instead.

Comment 17

[Steven Michaud [:smichaud] (Retired)]

Actually the failure probably happens somewhere in the following Mac-specific C++ code:

https://hg.mozilla.org/mozilla-central/annotate/f7a27a866c47/browser/components/shell/nsMacShellService.cpp#l68

So this may end up being "my" bug, after all.  But I may not be able to get to it until next week.

Comment 18

[:Gijs (he/him)]

This is annoying. The call to ::LSSetDefaultHandlerForURLScheme(CFSTR("https"), firefoxID) fails with a permissions error (OSStatus -54, permission error on file open) presumably because the call for "http" opens the permissions database and keeps it open until the dialog has gone away. Which is beautiful, but after much looking in Apple's documentation, I couldn't find an alternative API call to use.

Looking at the internals of the registry:

/System/Library/Frameworks/CoreServices.framework/Versions/A/Frameworks/LaunchServices.framework/Versions/A/Support/lsregister -dump | grep -n7 'scheme.*http'

(cribbed from somewhere on stackoverflow, the link is gone by now)

shows that both http and https handlers get set after the call to set http. Ditto for .html handling and friends. So if we get this -54 error on yosemite, it seems we can safely bail and assume everything else works (or doesn't, if the user says "no" to the prompt, but that is then also fine).

So then I wanted to make the shellservice not throw ns_error_failure if it encountered this error on yosemite. Unfortunately, it seems I can't use nsCocoaFeatures to get nsCocoaFeatures::OnYosemiteOrLater() because it fails to link. I'm too tired to understand why right now - more tomorrow.

Comment 19

[Steven Michaud [:smichaud] (Retired)]

When I have time I can reverse-engineer why the call to ::LSSetDefaultHandlerForURLScheme(CFSTR("https"), firefoxID) fails.  But that probably won't be until next week.  (I'll use nm -pam, class-dump, my interpose library from http://people.mozilla.org/~stmichaud/ReverseEngineering/InterposeLibraryTemplate/ and the Hopper Disassembler from http://www.hopperapp.com/.)

But in the meantime, Gijs, feel free to keep looking for a workaround.

Comment 20

[Steven Michaud [:smichaud] (Retired)]

I did some reverse engineering work on this today.  Basically (and not surprisingly) this is an Apple bug.

The "change your default web browser" dialog is presented as the result of a call to the following internal (non-exported) LaunchServices method:

OSStatus _LSPresentSchemeHandlerChangeUI(char *scheme, char *bundle_id, bool arg3, void *arg4);

This method returns immediately, before the user has had a chance to dismiss the dialog, and possibly even before the dialog is displayed.  (In fact the dialog gets displayed via XPC by a different process -- CoreServicesUIAgent.app in /System/Library/CoreServices/.)

As best I can tell, this dialog only gets displayed if you try to change the default handler for either the http or https schemes.  And if the user allows it in the resulting dialog, the default handler for both schemes gets changed (as best I can tell), regardless of which of these two schemes you originally made the call on.

So here's how we may be able to work around this bug on Yosemite:

1) Put the "if (aClaimAllTypes) {}" block before the calls to LSSetDefaultHandlerForURLScheme() for the http and https schemes.

2) Only call LSSetDefaultHandlerForURLScheme() for one of the two "sensitive" schemes -- presumably http.

Apple may fix this bug before the Yosemite release.  But it's unwise to count on that.

Next week I'll make a detailed bug report to Apple.

Comment 21

[Steven Michaud [:smichaud] (Retired)]

Yosemite DP8 just appeared.  You can upgrade to it from DP7.

It doesn't fix this bug.

Comment 22

[Steven Michaud [:smichaud] (Retired)]

> As best I can tell, this dialog only gets displayed if you try to
> change the default handler for either the http or https schemes.

Oops, this seems to be wrong.  Looking at the assembly code for the
following (undocumented) method, it looks like you only see that
dialog if you try to change the handler for the "http" scheme:

bool _LSShouldPromptForSchemeHandlerChange(CFStringRef scheme);

Comment 23

[:Gijs (he/him)]

I filed bug 1071749 for the shell service, and aim to have a patch here either today or tomorrow to fix the shell service callers to try...catch and report the relevant exceptions to the browser console, instead of breaking as they do now.

Comment 24

[:Gijs (he/him)]

Attachment: should catch shell service exceptions,

The difference between the in-content and non-incontent prefs is because one has 'Cu' defined and one doesn't.

Comment 25

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8494571 [details] [diff] [review]
should catch shell service exceptions,

Review of attachment 8494571 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine as a workaround. There are a few other places in the codebase that setDefaultBrowser is used, but having their functions not throw doesn't seem to be required, so I think we're good there.

I just have one question - see below. If the answer to my question is, as I suspect, "no", then r=me.

Thanks Gijs, smichaud.

::: browser/components/preferences/in-content/main.js
@@ +656,2 @@
>      let selectedIndex =
>        shellSvc.isDefaultBrowser(false, true) ? 1 : 0;

I've skimmed this bug, and the work that you and smichaud have done, and I suspect the answer to this question is "no", but is there any change of shellSvc.isDefaultBrowser also throwing? If so, I wonder if we should return early after reporting the error, like we do if there's no shellSvc.

::: browser/components/preferences/main.js
@@ +508,2 @@
>      let selectedIndex =
>        shellSvc.isDefaultBrowser(false, true) ? 1 : 0;

Same question as above.

Comment 26

[:Gijs (he/him)]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #25)
> Comment on attachment 8494571 [details] [diff] [review]
> should catch shell service exceptions,
> 
> Review of attachment 8494571 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks fine as a workaround. There are a few other places in the
> codebase that setDefaultBrowser is used, but having their functions not
> throw doesn't seem to be required, so I think we're good there.
> 
> I just have one question - see below. If the answer to my question is, as I
> suspect, "no", then r=me.
> 
> Thanks Gijs, smichaud.
> 
> ::: browser/components/preferences/in-content/main.js
> @@ +656,2 @@
> >      let selectedIndex =
> >        shellSvc.isDefaultBrowser(false, true) ? 1 : 0;
> 
> I've skimmed this bug, and the work that you and smichaud have done, and I
> suspect the answer to this question is "no", but is there any change of
> shellSvc.isDefaultBrowser also throwing? If so, I wonder if we should return
> early after reporting the error, like we do if there's no shellSvc.

On Linux, no. On Mac and Windows, it's theoretically possible. I'll get you an updated patch with that addressed.

Comment 27

[:Gijs (he/him)]

Attachment: should catch shell service exceptions,

Now with early returns.

Comment 28

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8495999 [details] [diff] [review]
should catch shell service exceptions,

Review of attachment 8495999 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks Gijs.

Comment 29

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/240297b949aa

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/240297b949aa

Comment 31

[:Gijs (he/him)]

Comment on attachment 8495999 [details] [diff] [review]
should catch shell service exceptions,

Approval Request Comment
[Feature/regressing bug #]: n/a
[User impact if declined]: on OS X yosemite, default browser notification bar won't disappear when selecting default browser
[Describe test coverage new/current, TBPL]: none
[Risks and why]: very low, bunch of try...catches
[String/UUID change made/needed]: none

Comment 32

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8495999 [details] [diff] [review]
should catch shell service exceptions,

Aurora+

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/87a153443769

Comment 34

[Catalin Varga [QA][:VarCat]]

Verified as fixed using the following evironment:

FF34 Build Id:20141002004001
FF35 Build Id:20141001030205
OS: Mac OS X 10.10