1293593 - <input type="color"> crashes Firefox on OS X

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect, critical)

Description

[danburzo]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:48.0) Gecko/20100101 Firefox/48.0
Build ID: 20160726073904

Steps to reproduce:

User agent: "Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:48.0) Gecko/20100101 Firefox/48.0"
OS X version: 10.11.6

1. Start with a HTML page that simply contains an <input type='color'>
2. Click on the color picker and in the OS X color picker switch to the "Color Sliders" tab
3. Try to drag the Brightness knob, repeatedly

JSFiddle for convenience: https://jsfiddle.net/wp25dr9d/




Actual results:

Browser crashes

Comment 1

[Mats Palmgren (:mats)]

bp-dc9f70d7-15cc-41d4-9f42-118ad2160820

The bug is in Cocoa?

Comment 2

[Stephen A Pohl [:spohl] (OOO 1/21 - 1/26)]

Able to reproduce and I'm pretty confident I know what's going on. Waiting for clobber build to complete to confirm the patch.

Comment 3

[Stephen A Pohl [:spohl] (OOO 1/21 - 1/26)]

Attachment: Patch

In the steps to reproduce in comment 0, note that you may have to select "Gray Scale Slider" (or similar) between steps 2 and 3 from the drop down menu. I've been able to crash Firefox with "Gray Scale Slider" selected and then confirmed that this patch fixes the issue for all options in the drop down menu.

This crash occurs because [NSColor getRed:green:blue:alpha] throws an (uncaught) NSException* when called on an NSColor instance with an incompatible NSColorSpace (see "Class at a Glance" at [1]). Note that we can't use |respondsToSelector:| to determine whether or not it will throw an exception.

I moved the conversion of the color space from |nsColorPicker::Update| to |nsColorPicker::GetHexStringFromNSColor| to get it closer to the call to [NSColor getRed:green:blue:alpha] and make it easier to catch and handle the exception if one occurs.

Side note: In bug 1247373, after doing the color conversion, we accidentally continued to pass |aColor| instead of |color| to |nsColorPicker::GetHexStringFromNSColor|. This crash would still have occurred though, since the color space with the "Gray Scale Slider" selected is NSCalibratedWhiteColorSpace.

[1] https://developer.apple.com/library/mac/documentation/Cocoa/Reference/ApplicationKit/Classes/NSColor_Class/

Comment 4

[Markus Stange [:mstange]]

Comment on attachment 8783151 [details] [diff] [review]
Patch

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

Oops!

Comment 5

[Stephen A Pohl [:spohl] (OOO 1/21 - 1/26)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/66d066d5f608c969d486c310d297d2c7fac79aaa
Bug 1293593: Fix crash due to an Objective-C exception when calling getRed:green:blue:alpha on an NSColor instance with an incompatible NSColorSpace. r=mstange

Comment 6

[Ryan VanderMeulen [:RyanVM][PTO Jan 25 - Feb 2]]

https://hg.mozilla.org/mozilla-central/rev/66d066d5f608

Comment 7

[Ritu Kothari (:ritu) (On PTO, n-i to :jcristau, :ryanvm, :lizzard instead)]

Hello Stephen, could you please nominate for uplift to Aurora50 and Beta49?

Comment 8

[Stephen A Pohl [:spohl] (OOO 1/21 - 1/26)]

Comment on attachment 8783151 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/regressing bug #]: n/a
[User impact if declined]: Using the color picker on OSX may crash the browser.
[Describe test coverage new/current, TreeHerder]: Verified locally.
[Risks and why]: Virtually none. We add a try/catch block to catch a native exception that previously crashed the browser. All instances that didn't cause an exception continue to work as before.
[String/UUID change made/needed]: none

Comment 9

[Ritu Kothari (:ritu) (On PTO, n-i to :jcristau, :ryanvm, :lizzard instead)]

Hello Dan, could you please verify this issue is fixed as expected on Nightly build 8/23/2016? Thanks!

Comment 10

[danburzo]

I can confirm this is now working as expected on Firefox Nightly. Thank you, everyone!

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8783151 [details] [diff] [review]
Patch

Fix a crash, taking it.
Should be in 49 beta 7

Comment 12

[Ritu Kothari (:ritu) (On PTO, n-i to :jcristau, :ryanvm, :lizzard instead)]

(In reply to danburzo from comment #10)
> I can confirm this is now working as expected on Firefox Nightly. Thank you,
> everyone!

Fantastic! Thanks a lot for a prompt follow up.

Comment 13

[Ryan VanderMeulen [:RyanVM][PTO Jan 25 - Feb 2]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/41d9edca6780

Comment 14

[Ryan VanderMeulen [:RyanVM][PTO Jan 25 - Feb 2]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c56047b7ac0d

Comment 15

[Paul Silaghi, QA [:pauly]]

Verified fixed FX 49b8, 50.0a2 (2016-09-01) OS X 10.11.