1389908 - macOS: HTML color picker crashes Firefox

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Sören Hentzschel]

Attachment: new_tab_override-8.0.0.xpi

Firefox crashes very often (not always) after clicking on the HTML color picker. Here are a few crash reports:

https://crash-stats.mozilla.com/report/index/34f34ab5-1e03-4b8b-a73b-2ff351170813
https://crash-stats.mozilla.com/report/index/00714bb0-ad3f-4475-aaec-aba9f1170813
https://crash-stats.mozilla.com/report/index/8140c8a8-30bc-4055-af9c-6a26b1170813
https://crash-stats.mozilla.com/report/index/5d549580-153b-4124-ab2b-b15911170813

I attached the development version of my add-on New Tab Override, version 8.0. In the addon's setting you can chose the option "background color" and use this to test.

macOS High Sierra 10.13 Developer Beta 5

Comment 1

[Sören Hentzschel]

It seems to always work on the first use and always to crash on the second use.

Comment 2

[Arnaud Bienner]

I will try to have a look.
I remember we had some issues on macOS with multiple color pickers, so we kept only one instance at a time (see bug 975468).
I'm wondering if the crash could be the result of a similar cause.
I suspect it's limited only to addons, since AFAICT we had no crash reported when using <input type=color> multiple times on macOS since.

Comment 3

[Arnaud Bienner]

It's working for me using the latest sources from mozilla-central, on El Capitan 10.11.6.
You mentioned you're using a beta version of macOS: I'm wondering if that couldn't be a bug on macOS then.
Did you test on another version of macOS?
Did you noticed a similar bug/crash on other applications using a color picker?

Side note: I saw your addon has a URL field: you can use input type=url instead of type=text ;)

Comment 4

[Sören Hentzschel]

Thanks for looking into the issue!

> You mentioned you're using a beta version of macOS: I'm wondering if that couldn't be a bug on macOS then.
> Did you test on another version of macOS?

Seems to be a bug on the Beta of macOS High Sierra! On 10.11.4 I cannot reproduce.

> Did you noticed a similar bug/crash on other applications using a color picker?

I have to check later (I am at work and there is only macOS 10.11, at home I only have macOS 10.13 Beta).

> Side note: I saw your addon has a URL field: you can use input type=url instead of type=text ;)

Good suggestion, thank you. ;)

Comment 5

[Sören Hentzschel]

(In reply to Arnaud Bienner from comment #2)
> I suspect it's limited only to addons, since AFAICT we had no crash reported
> when using <input type=color> multiple times on macOS since.

Tested. It also crashes on web content.

(In reply to Arnaud Bienner from comment #3)
> Did you noticed a similar bug/crash on other applications using a color
> picker?

No crash on Google Chrome and Apple Safari.

Comment 6

[Sören Hentzschel]

Updated to macOS High Sierra 10.13 Developer Beta 6, it's still an issue.

Comment 7

[Arnaud Bienner]

Unfortunately I don't have macOS High Sierra and I can't afford to install a developer version on my mac book.
Not sure who can have a look at this...
Changing component from "form control" to "widget: cocoa" since this is more related to the color picker widget implementation.

Comment 8

[Arnaud Bienner]

Markus: according to the module page [1] you're the owner of the cocoa related stuff.
Do you think you can have a look at this macOS High Sierra specific issue?

The crash seems to affect only this version of macOS. I'm not sure if this is something that would be fixed within macOS before High Sierra get released, or if this is really something wrong on our implementation.

Let me know if you have any question about the current implementation of the color picker.

[1]: https://wiki.mozilla.org/Modules/All#Core

Comment 9

[Markus Stange [:mstange]]

I was going to find out if this will be fixed by updating the SDK (bug 1324892) but ran into bug 1392431 in the process.

Comment 10

[Markus Stange [:mstange]]

When NSColorPanelWrapper is deallocated, the delegate on the NSColorPanel is already nil (don't know why), so we don't call [mColorPanel setTarget:nil] on the color panel, so its target pointer becomes dangling.
When we then call setColor on it, it wants to notify the old target, and crashes when accessing the (now invalid) target pointer.

Comment 11

[Markus Stange [:mstange]]

Attachment: Bug 1389908 - Make sure the NSColorPanel never has a dangling target, and make sure to set the new target before calling setColor.

Review commit: https://reviewboard.mozilla.org/r/170930/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/170930/

Comment 12

[Stephen A Pohl [:spohl]]

Comment on attachment 8899623 [details]
Bug 1389908 - Make sure the NSColorPanel never has a dangling target, and make sure to set the new target before calling setColor.

https://reviewboard.mozilla.org/r/170930/#review176108

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6d2fe35e5c63
Make sure the NSColorPanel never has a dangling target, and make sure to set the new target before calling setColor. r=spohl

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/6d2fe35e5c63

Comment 15

[Gerry Chang [:gchang]]

Track 56+ as crashes on macOS.

Comment 16

[Sören Hentzschel]

I can confirm that it's fixed in today's Nightly. Thank you!

Comment 17

[Markus Stange [:mstange]]

Comment on attachment 8899623 [details]
Bug 1389908 - Make sure the NSColorPanel never has a dangling target, and make sure to set the new target before calling setColor.

Approval Request Comment
[Feature/Bug causing the regression]: macOS 10.13
[User impact if declined]: crashes when opening the color picker a second time
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: it's simple
[String changes made/needed]: none

Comment 18

[Gerry Chang [:gchang]]

Comment on attachment 8899623 [details]
Bug 1389908 - Make sure the NSColorPanel never has a dangling target, and make sure to set the new target before calling setColor.

Fix a crash when opening the color picker on the second use and was verified. Beta56+.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

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

Comment 20

[Alexandru Simonca [:asimonca], Release Desktop QA]

Hi,
Due to the fact that Beta does not support unsigned xpi's I've tested this issue by enabling add-on debugging in about:debugging and using the "Load Temporary Add-on" button to load the add-on from the attachments in the browser. I've opened the color picker and selected different colors and opened the respective tabs to see if they were applied. Nothing crashed so far. 
I consider this issue VERIFIED FIXED on Firefox Beta 56.0b6.
I used macOS 10.13 High Sierra Developer Beta 7 and a 2017 MacBook Pro with touchbar.
Please ni? me if this wasn't the best approach to testing this.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

[Tracking Requested - why for this release]:

We're spinning an ESR52 dot release for 10.13 issues. Is this something we should consider including in it? It grafts cleanly.

Comment 22

[Markus Stange [:mstange]]

Comment on attachment 8899623 [details]
Bug 1389908 - Make sure the NSColorPanel never has a dangling target, and make sure to set the new target before calling setColor.

Good idea, let's do it.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: simple fix which fixes a crash bug on 10.13
User impact if declined: crashes when opening the color picker a second time
Fix Landed on Version: 56
Risk to taking this patch (and alternatives if risky): extremely low
String or UUID changes made by this patch: none

Comment 23

[Julien Cristau [:jcristau]]

Comment on attachment 8899623 [details]
Bug 1389908 - Make sure the NSColorPanel never has a dangling target, and make sure to set the new target before calling setColor.

crash fix for latest macos, ride along for 52.4.1.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/e4fec62e5347

Comment 25

[Oana Botisan, Desktop QA]

I managed to reproduce the bug on an old Nightly (2016-06-23) using macOS 10.13. I installed the add-on New Tab Override and after I changed the background color the browser crashed.

I tried to test it on esr 52.4.1 - build 1, but unfortunately I couldn't install this add-on neither by recreating the steps from comment 20 nor changing the prefs in about:config. An error that the file is corrupted was displayed.  

On the other hand I managed to find a similar add-on (https://addons-dev.allizom.org/en-US/firefox/addon/theme-font-size-changer/), which does almost the same thing. When I changed the colors the browser didn't crash. But the problem is that I couldn't verify that the bug reproduces with this add-on on older versions of Firefox because they are not compatible.

Can you please suggest another add-on that might work? Or it's alright the one that I found?

Comment 26

[Markus Stange [:mstange]]

Attachment: testcase

I think the crash was reproducing with this testcase.

Comment 27

[Oana Botisan, Desktop QA]

(In reply to Markus Stange [:mstange] from comment #26)
> Created attachment 8916762 [details]
> testcase
> 
> I think the crash was reproducing with this testcase.

I tried to reproduce the bug with the app you attached to your comment and a few other add-ons (like ColorZilla) using an Nightly from 2017-08-13, but I couldn't. 
I even tried to change the colors in the browser from about:preferences thinking that this could be the reason the browser crashes, but nothing happen. The reason I thought that is because the first time I manage to reproduce the bug with the steps from comment 0, the browser crashed only after I opened a new tab with the background's color changed. 

Do you have any more ideas what I could try? I ran out of them.

Comment 28

[Sören Hentzschel]

(clearing old needinfo)