1286490 - [Elevated Update] Fast Elevation Cancel hangs Firefox on Mac OS X

Status: VERIFIED FIXED

(Toolkit :: Application Update, defect)

Description

[Bogdan Maris, Desktop QA]

[Note]:
- This is a followup of bug 1274319

[Affected versions]:
- Developer Edition 49.0a2
- Nightly 50.0a1

[Affected platforms]:
- Mac OS X 10.9

[Steps to reproduce]:
1. Install DevEdition from a few days ago from Admin account.
2. Switch to standard account.
3. Check for update, download and restart to update.
4. When the elevated authentication window is displayed press a fast Cancel.

If Firefox starts repeat steps 3-4. (I usually reproduce when I cancel the second time).

[Expected result]:
- Firefox restarts after canceling the login window.

[Actual result]:
- Firefox hangs after canceling the login window.

[Regression range]:
- This is not a regression, it reproduced on old builds as well and bug 1274319 never fixed it on 10.9.

[Additional notes]:
- Message I get from terminal, when hang occurs:

> sl115-32:~ standard1$ 2016-07-11 11:43:15.013 firefox[715:303] AuthorizationCopyRights failed! NSOSStatusErrorDomain / -60006
> 2016-07-11 11:43:15.032 firefox[715:303] Unable to clean up updater.

Comment 1

[Stephen A Pohl [:spohl]]

When Firefox is in the bad state, could you run the following three commands and tell me what gets printed to the console for each one:

sudo launchctl list | grep org.mozilla.updater
sudo ps ax | grep org.mozilla.updater
sudo ps ax | grep firefox

Thanks!

Comment 2

[Bogdan Maris, Desktop QA]

(In reply to Stephen A Pohl [:spohl] from comment #1)
> When Firefox is in the bad state, could you run the following three commands
> and tell me what gets printed to the console for each one:
> 
> sudo launchctl list | grep org.mozilla.updater
> sudo ps ax | grep org.mozilla.updater
> sudo ps ax | grep firefox
> 
> Thanks!

Sure, here is the terminal output: http://pastebin.com/XiFXs93P
Not sure if this helps or not.

Comment 3

[Stephen A Pohl [:spohl]]

Attachment: Improve logging

The timestamps in comment 0 between "AuthorizationCopyRights failed" and "Unable to clean up updater!" are in very fast succession, which seems to indicate that we hit an NSException and we don't actually retry the cleanup attemp ten times. Before I rewrite the try/catch block to retry even when we hit an NSException, I'd like to see what kind of exception we're actually hitting. I'll fix the actual issue in a subsequent patch once we know more.

Comment 4

[Stephen A Pohl [:spohl]]

Setting checking-needed since inbound is currently closed.

Comment 5

[Stephen A Pohl [:spohl]]

Please retest this after the patch has been on nightly for a couple of days. Once the issue reproduces, please copy the console output from Terminal or Console in a new comment like you did in comment 0. Thanks!

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/9b1f6f45c1d7
Improve logging when unable to clean up elevated updater on OSX. r=mstange

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9b1f6f45c1d7

Comment 8

[Bogdan Maris, Desktop QA]

Here is the output of the Terminal while fast canceling an update from Nightly (2016-08-23) using Mac OS X 10.9.5, 10.10.5 and 10.11.1 (I reproduced on all of them).

> SL115-136:~ standard$ /Applications/FirefoxNightly.app/Contents/MacOS/firefox -no-remote -jsconsole
> 
> 2016-08-25 17:25:34.216 firefox[2671:141946] AuthorizationCopyRights failed! NSOSStatusErrorDomain / -60006
> SL115-136:~ standard$ 2016-08-25 17:25:51.583 firefox[2685:142488] AuthorizationCopyRights failed! NSOSStatusErrorDomain / -60006
> 
> 2016-08-25 17:25:51.605 firefox[2685:142488] NSPortTimeoutException: Distributed objects message send timed out (timeout: 10493827951.594898 at time: 493827951.595064) 1
> 
> 2016-08-25 17:25:51.605 firefox[2685:142488] Unable to clean up updater.

Comment 9

[Stephen A Pohl [:spohl]]

Attachment: Patch

Ok, this patch should take care of it. Thanks!

Comment 10

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/435841ba617b50c2c59296694db1103e0d0debb6
Bug 1286490: Handle IPC timeout exceptions during elevated updates on OSX. r=mstange

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/435841ba617b

Comment 12

[Stephen A Pohl [:spohl]]

Bogdan, could you verify that this is indeed fixed now? Thanks!

Comment 13

[Bogdan Maris, Desktop QA]

Just verified on Mac OS X 10.10.5, 10.11.1 and 10.9.5 canceling the update from old Nightly (2016-08-27) to latest Nightly did not hang Firefox no matter how many times I canceled the update. 

Will this be uplifted to beta as well? It could be nice to have this in 49 when it reaches release.

Comment 14

[Stephen A Pohl [:spohl]]

Attachment: Patch for uplift

Thanks, Bogdan! Not sure if we can uplift all the way, but I can certainly request it and see.

Approval Request Comment
[Feature/regressing bug #]: Bug 394984
[User impact if declined]: Fast cancellations of the elevation prompt during an elevated update on OSX can hang Firefox.
[Describe test coverage new/current, TreeHerder]: Verified by QA.
[Risks and why]: Minimal to none. Very isolated and well understood patch.
[String/UUID change made/needed]: none

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8785960 [details] [diff] [review]
Patch for uplift

Fixes a hang in a corner case, fix was verified on Nightly, Aurora50+

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7034b14c71ba

Comment 17

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8785960 [details] [diff] [review]
Patch for uplift

Maybe an edge case but we verified the fix and I'd like to make sure as many people can update smoothly as possible.  Taking this for beta 9.

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/33fa756e769e

Comment 19

[Andrei Vaida [:avaida]]

We should verify this on 49.0b9 as well. Bogdan, could you please have a look?

Comment 20

[Bogdan Maris, Desktop QA]

QA [:avaida] – please ni? me from comment #19)
> We should verify this on 49.0b9 as well. Bogdan, could you please have a
> look?

Sure. I just finished verifying this across different Mac versions (10.11.1, 10.10.5 and 10.9.5) using tinderbox mozilla-beta build and updated to latest Firefox 49 beta 9 using 'beta-cdntest' update chanel. I'll mark this as verified in 50 as well since I tested on Aurora as well a few weeks ago.(In reply to Andrei Vaida,