Closed Bug 1389908 Opened 7 years ago Closed 7 years ago

macOS: HTML color picker crashes Firefox

Categories

(Core :: Widget: Cocoa, defect)

55 Branch
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla57
Tracking Status
firefox-esr52 56+ fixed
firefox55 --- wontfix
firefox56 + verified
firefox57 + verified

People

(Reporter: soeren.hentzschel, Assigned: mstange)

References

Details

Attachments

(3 files)

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
It seems to always work on the first use and always to crash on the second use.
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.
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 ;)
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. ;)
(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.
Blocks: highsierra
Updated to macOS High Sierra 10.13 Developer Beta 6, it's still an issue.
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.
Component: Layout: Form Controls → Widget: Cocoa
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
Flags: needinfo?(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.
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.
Assignee: nobody → mstange
Status: NEW → ASSIGNED
Flags: needinfo?(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.

https://reviewboard.mozilla.org/r/170930/#review176108
Attachment #8899623 - Flags: review?(spohl.mozilla.bugs) → review+
Pushed by mstange@themasta.com:
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
https://hg.mozilla.org/mozilla-central/rev/6d2fe35e5c63
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Track 56+ as crashes on macOS.
I can confirm that it's fixed in today's Nightly. Thank you!
Status: RESOLVED → VERIFIED
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
Attachment #8899623 - Flags: approval-mozilla-beta?
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+.
Attachment #8899623 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
QA Contact: alexandru.simonca
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.
[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.
Flags: needinfo?(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
Flags: needinfo?(mstange)
Attachment #8899623 - Flags: approval-mozilla-esr52?
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.
Attachment #8899623 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
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?
Flags: needinfo?(cadeyrn)
Attached file testcase
I think the crash was reproducing with this testcase.
(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.
(clearing old needinfo)
Flags: needinfo?(cadeyrn)
You need to log in before you can comment on or make changes to this bug.