Closed Bug 1293593 Opened 4 years ago Closed 4 years ago

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

Categories

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

48 Branch
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla51
Tracking Status
firefox48 --- wontfix
firefox49 --- verified
firefox50 --- verified
firefox51 --- verified

People

(Reporter: danburzo, Assigned: spohl)

References

Details

(Keywords: crash, crashreportid)

Crash Data

Attachments

(1 file)

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
bp-dc9f70d7-15cc-41d4-9f42-118ad2160820

The bug is in Cocoa?
Severity: normal → critical
Status: UNCONFIRMED → NEW
Component: Layout: Form Controls → Widget: Cocoa
Ever confirmed: true
Keywords: crash, crashreportid
Able to reproduce and I'm pretty confident I know what's going on. Waiting for clobber build to complete to confirm the patch.
Assignee: nobody → spohl.mozilla.bugs
Status: NEW → ASSIGNED
Attached patch PatchSplinter Review
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/
Attachment #8783151 - Flags: review?(mstange)
See Also: → 1247373
Comment on attachment 8783151 [details] [diff] [review]
Patch

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

Oops!
Attachment #8783151 - Flags: review?(mstange) → review+
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
https://hg.mozilla.org/mozilla-central/rev/66d066d5f608
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Hello Stephen, could you please nominate for uplift to Aurora50 and Beta49?
Flags: needinfo?(spohl.mozilla.bugs)
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
Flags: needinfo?(spohl.mozilla.bugs)
Attachment #8783151 - Flags: approval-mozilla-beta?
Attachment #8783151 - Flags: approval-mozilla-aurora?
Hello Dan, could you please verify this issue is fixed as expected on Nightly build 8/23/2016? Thanks!
Flags: needinfo?(danburzo)
I can confirm this is now working as expected on Firefox Nightly. Thank you, everyone!
Flags: needinfo?(danburzo)
Crash Signature: [@ libobjc.A.dylib@0x14e9]
Comment on attachment 8783151 [details] [diff] [review]
Patch

Fix a crash, taking it.
Should be in 49 beta 7
Attachment #8783151 - Flags: approval-mozilla-beta?
Attachment #8783151 - Flags: approval-mozilla-beta+
Attachment #8783151 - Flags: approval-mozilla-aurora?
Attachment #8783151 - Flags: approval-mozilla-aurora+
(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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Verified fixed FX 49b8, 50.0a2 (2016-09-01) OS X 10.11.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.