1543813 - Crash in [@ _platform_memmove$VARIANT$Haswell | nsTSubstring<T>::StartBulkWriteImpl]

Status: RESOLVED FIXED

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

Description

[Marcia Knous [:marcia]]

This bug is for crash report bp-000c23ca-0454-4d8f-a2f7-6fe350190410.

Seen while looking at Mac crash stats - crashes go back to earlier releases. Almost all of the comments mention crashing when picking the color picker: https://bit.ly/2Kv5qUd

• Crashed when using the color picker to change the visited link color in about:preferences
• Choosing colors for the Text Highlighter add on
• When closing the colour picker, Firefox crashes.

Some URLs:

• https://mapstyle.withgoogle.com/
• http://stephenhorlander.com/collider/firefox-monitor.html
• https://cloud.maptiler.com/customize/#bright/1.28/0/0

Top 10 frames of crashing thread:

0 libsystem_platform.dylib _platform_memmove$VARIANT$Haswell 
1 XUL nsTSubstring<char16_t>::StartBulkWriteImpl xpcom/string/nsTSubstring.cpp:245
2 XUL nsTSubstring<char16_t>::SetLength xpcom/string/nsTSubstring.cpp:949
3 XUL nsCocoaUtils::GetStringForNSString widget/cocoa/nsCocoaUtils.mm:535
4 XUL -[NSColorPanelWrapper colorChanged:] widget/cocoa/nsColorPicker.mm:61
5 AppKit -[NSApplication sendAction:to:from:] 
6 AppKit __61-[NSColorPanel _forceSendAction:notification:firstResponder:]_block_invoke 
7 AppKit -[NSColorPanel _withColorSettingDisabled:] 
8 AppKit -[NSColorPanel _forceSendAction:notification:firstResponder:] 
9 AppKit -[NSHSBSliders _adjustControls:andSetColor:] 

Comment 1

[Markus Stange [:mstange]]

It looks like NSColorPanelWrapper's mColorPicker field points at a destroyed nsColorPicker object.

I tried to understand the reference counting setup in nsColorPicker.mm and failed. This is from a patch I reviewed 6 years ago... today I would call its ownership setup "rather adventurous".

Comment 2

[Pascal Chevrel:pascalc (PTO until Sept 2)]

5 crashes in 66, 3 in 67 beta. We wouldn't uplift fixes for such low volume, wontfix for the releases in flight.

Comment 3

[Marcia Knous [:marcia]]

https://bit.ly/2H6xUQp - another Mac signature that has one comment that mentions the color picker - but the stack is a bit different.

Comment 4

[Patricia Lawless]

Bulk change of P3 carryover bugs to wontfix for 68.

Comment 6

[Stephen A Pohl [:spohl]]

I am going to post patches shortly, but figured I'd write up an analysis here.

At the heart of the problem here is the fact that typically, AppKit sends us one and only one action to change the color as follows:

(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1
  * frame #0: 0x000000011bc426fd XUL`-[NSColorPanelWrapper colorChanged:](self=0x000000010f0989e0, _cmd="colorChanged:", aPanel=0x0000000113577000) at nsColorPicker.mm:62:3
    frame #1: 0x00007fff338de437 AppKit`-[NSApplication(NSResponder) sendAction:to:from:] + 299
    frame #2: 0x00007fff33bc0c78 AppKit`__61-[NSColorPanel _forceSendAction:notification:firstResponder:]_block_invoke + 100
    frame #3: 0x00007fff33bc0ae6 AppKit`-[NSColorPanel _withColorSettingDisabled:] + 37
    frame #4: 0x00007fff33bc0c0e AppKit`-[NSColorPanel _forceSendAction:notification:firstResponder:] + 83
    frame #5: 0x000000011bc4269d XUL`-[NSColorPanelWrapper open:title:](self=0x000000010f0989e0, _cmd="open:title:", aInitialColor=0xb8bd02526ebfff55, aTitle="Choose a color") at nsColorPicker.mm:56:3
    frame #6: 0x000000011bc4350c XUL`nsColorPicker::Open(this=0x000000010f0aa800, aCallback=0x000000014c6470c0) at nsColorPicker.mm:146:3
    frame #7: 0x000000011b305646 XUL`mozilla::dom::ColorPickerParent::RecvOpen(this=0x00000001497064c0) at ColorPickerParent.cpp:68:12
    frame #8: 0x0000000115ce130b XUL`mozilla::dom::PColorPickerParent::OnMessageReceived(this=0x00000001497064c0, msg__=0x000000014889f440) at PColorPickerParent.cpp:136:28
    frame #9: 0x0000000115dde7c8 XUL`mozilla::dom::PContentParent::OnMessageReceived(this=0x0000000149867000, msg__=0x000000014889f440) at PContentParent.cpp:6376:32
    frame #10: 0x0000000115b224fc XUL`mozilla::ipc::MessageChannel::DispatchAsyncMessage(this=0x0000000149867130, aProxy=0x0000000149853ee0, aMsg=0x000000014889f440) at MessageChannel.cpp:2187:25
    frame #11: 0x0000000115b203f8 XUL`mozilla::ipc::MessageChannel::DispatchMessage(this=0x0000000149867130, aMsg=0x000000014889f440) at MessageChannel.cpp:2111:9
    frame #12: 0x0000000115b20f8e XUL`mozilla::ipc::MessageChannel::RunMessage(this=0x0000000149867130, aTask=0x000000014889f3e0) at MessageChannel.cpp:1959:3
    frame #13: 0x0000000115b2173f XUL`mozilla::ipc::MessageChannel::MessageTask::Run(this=0x000000014889f3e0) at MessageChannel.cpp:1990:13
    frame #14: 0x0000000114b16056 XUL`nsThread::ProcessNextEvent(this=0x000000010f076350, aMayWait=false, aResult=0x00007ffee54c1f4f) at nsThread.cpp:1200:14
    frame #15: 0x0000000114b120bc XUL`NS_ProcessPendingEvents(aThread=0x000000010f076350, aTimeout=10) at nsThreadUtils.cpp:429:19
    frame #16: 0x000000011bb5768c XUL`nsBaseAppShell::NativeEventCallback(this=0x00000001101fd3a0) at nsBaseAppShell.cpp:87:3
    frame #17: 0x000000011bc164b5 XUL`nsAppShell::ProcessGeckoEvents(aInfo=0x00000001101fd3a0) at nsAppShell.mm:427:11
    frame #18: 0x00007fff3641ef12 CoreFoundation`__CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17
    frame #19: 0x00007fff3641eeb1 CoreFoundation`__CFRunLoopDoSource0 + 103
    frame #20: 0x00007fff3641eccb CoreFoundation`__CFRunLoopDoSources0 + 209
    frame #21: 0x00007fff3641d9fa CoreFoundation`__CFRunLoopRun + 927
    frame #22: 0x00007fff3641cffe CoreFoundation`CFRunLoopRunSpecific + 462
    frame #23: 0x00007fff35050abd HIToolbox`RunCurrentEventLoopInMode + 292
    frame #24: 0x00007fff350507d5 HIToolbox`ReceiveNextEventCommon + 584
    frame #25: 0x00007fff35050579 HIToolbox`_BlockUntilNextEventMatchingListInModeWithFilter + 64
    frame #26: 0x00007fff3369bc99 AppKit`_DPSNextEvent + 883
    frame #27: 0x00007fff3369a4e0 AppKit`-[NSApplication(NSEvent) _nextEventMatchingEventMask:untilDate:inMode:dequeue:] + 1352
    frame #28: 0x000000011bc151db XUL`-[GeckoNSApplication nextEventMatchingMask:untilDate:inMode:dequeue:](self=0x000000010ac4f660, _cmd="nextEventMatchingMask:untilDate:inMode:dequeue:", mask=18446744073709551615, expiration=0x00007fff8dfa3218, mode="kCFRunLoopDefaultMode", flag=YES) at nsAppShell.mm:169:24
    frame #29: 0x00007fff3368c1ee AppKit`-[NSApplication run] + 658
    frame #30: 0x000000011bc1728e XUL`nsAppShell::Run(this=0x00000001101fd3a0) at nsAppShell.mm:690:5
    frame #31: 0x000000011ef633c0 XUL`nsAppStartup::Run(this=0x000000011037a2e0) at nsAppStartup.cpp:271:30
    frame #32: 0x000000011f179950 XUL`XREMain::XRE_mainRun(this=0x00007ffee54c41b8) at nsAppRunner.cpp:4690:22
    frame #33: 0x000000011f17aa0e XUL`XREMain::XRE_main(this=0x00007ffee54c41b8, argc=5, argv=0x00007ffee54c4840, aConfig=0x00007ffee54c4368) at nsAppRunner.cpp:4825:8
    frame #34: 0x000000011f17b0ab XUL`XRE_main(argc=5, argv=0x00007ffee54c4840, aConfig=0x00007ffee54c4368) at nsAppRunner.cpp:4879:21
    frame #35: 0x000000011f1963a7 XUL`mozilla::BootstrapImpl::XRE_main(this=0x000000010ac131e0, argc=5, argv=0x00007ffee54c4840, aConfig=0x00007ffee54c4368) at Bootstrap.cpp:45:12
    frame #36: 0x000000010a73c4be firefox`do_main(argc=5, argv=0x00007ffee54c4840, envp=0x00007ffee54c4870) at nsBrowserApp.cpp:217:22
    frame #37: 0x000000010a73bdb8 firefox`main(argc=5, argv=0x00007ffee54c4840, envp=0x00007ffee54c4870) at nsBrowserApp.cpp:331:16
    frame #38: 0x00007fff70365cc9 libdyld.dylib`start + 1

However, sometimes we receive an immediate, second action which seemingly originates from a text field as follows (see frame #10):

(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x000000011bb5d9e0 XUL`nsCOMPtr<nsIColorPickerShownCallback>::operator->(this=0x000000010f062e38) const at nsCOMPtr.h:858:5
    frame #1: 0x000000011bc42783 XUL`nsColorPicker::Update(this=0x000000010f062e00, aColor=0x82bd02526ebfff55) at nsColorPicker.mm:156:3
    frame #2: 0x000000011bc42729 XUL`-[NSColorPanelWrapper colorChanged:](self=0x0000000144c90580, _cmd="colorChanged:", aPanel=0x0000000113577000) at nsColorPicker.mm:62:17
    frame #3: 0x00007fff338de437 AppKit`-[NSApplication(NSResponder) sendAction:to:from:] + 299
    frame #4: 0x00007fff33bc0c78 AppKit`__61-[NSColorPanel _forceSendAction:notification:firstResponder:]_block_invoke + 100
    frame #5: 0x00007fff33bc0ae6 AppKit`-[NSColorPanel _withColorSettingDisabled:] + 37
    frame #6: 0x00007fff33bc0c0e AppKit`-[NSColorPanel _forceSendAction:notification:firstResponder:] + 83
    frame #7: 0x00007fff33bd515c AppKit`-[NSRGBSliders _adjustControls:andSetColor:] + 997
    frame #8: 0x00007fff338de437 AppKit`-[NSApplication(NSResponder) sendAction:to:from:] + 299
    frame #9: 0x00007fff338de2d2 AppKit`-[NSControl sendAction:to:] + 86
    frame #10: 0x00007fff339b04bb AppKit`-[NSTextField textDidEndEditing:] + 536
    frame #11: 0x00007fff364149cf CoreFoundation`__CFNOTIFICATIONCENTER_IS_CALLING_OUT_TO_AN_OBSERVER__ + 12
    frame #12: 0x00007fff36414963 CoreFoundation`___CFXRegistrationPost1_block_invoke + 63
    frame #13: 0x00007fff364148d8 CoreFoundation`_CFXRegistrationPost1 + 372
    frame #14: 0x00007fff36414544 CoreFoundation`___CFXNotificationPost_block_invoke + 80
    frame #15: 0x00007fff363e46bd CoreFoundation`-[_CFXNotificationRegistrar find:object:observer:enumerator:] + 1554
    frame #16: 0x00007fff363e3b69 CoreFoundation`_CFXNotificationPost + 1351
    frame #17: 0x00007fff38a5a866 Foundation`-[NSNotificationCenter postNotificationName:object:userInfo:] + 59
    frame #18: 0x00007fff339affc1 AppKit`-[NSTextView(NSSharing) resignFirstResponder] + 782
    frame #19: 0x00007fff337e9c6b AppKit`-[NSWindow _realMakeFirstResponder:] + 203
    frame #20: 0x00007fff338bb1e7 AppKit`-[NSWindow endEditingFor:] + 303
    frame #21: 0x00007fff339f66ef AppKit`-[NSPanel resignKeyWindow] + 48
    frame #22: 0x00007fff33bbc85d AppKit`-[NSColorPanel resignKeyWindow] + 74
    frame #23: 0x00007fff339500ed AppKit`-[NSWindow _orderOutAndCalcKeyWithCounter:stillVisible:docWindow:] + 256
    frame #24: 0x00007fff336f3fce AppKit`NSPerformVisuallyAtomicChange + 132
    frame #25: 0x00007fff3394ff39 AppKit`-[NSWindow _doWindowOrderOutWithWithKeyCalc:forCounter:orderingDone:docWindow:] + 80
    frame #26: 0x00007fff3394f77a AppKit`-[NSWindow _reallyDoOrderWindowOutRelativeTo:findKey:forCounter:force:isModal:] + 412
    frame #27: 0x00007fff337fac42 AppKit`-[NSWindow _reallyDoOrderWindow:relativeTo:findKey:forCounter:force:isModal:] + 183
    frame #28: 0x00007fff33a10d30 AppKit`__71-[NSWindow _doOrderWindow:relativeTo:findKey:forCounter:force:isModal:]_block_invoke + 410
    frame #29: 0x00007fff336f3fce AppKit`NSPerformVisuallyAtomicChange + 132
    frame #30: 0x00007fff337f9e17 AppKit`-[NSWindow _doOrderWindow:relativeTo:findKey:forCounter:force:isModal:] + 926
    frame #31: 0x00007fff337f9a17 AppKit`-[NSWindow orderWindow:relativeTo:] + 155
    frame #32: 0x00007fff33f3731f AppKit`-[NSWindow _finishClosingWindow] + 437
    frame #33: 0x00007fff33a12d05 AppKit`-[NSWindow _close] + 352
    frame #34: 0x00007fff33a6282f AppKit`-[NSWindow __close] + 292
    frame #35: 0x00007fff338de437 AppKit`-[NSApplication(NSResponder) sendAction:to:from:] + 299
    frame #36: 0x00007fff338de2d2 AppKit`-[NSControl sendAction:to:] + 86
    frame #37: 0x00007fff338de204 AppKit`__26-[NSCell _sendActionFrom:]_block_invoke + 136
    frame #38: 0x00007fff338de106 AppKit`-[NSCell _sendActionFrom:] + 171
    frame #39: 0x00007fff338de04d AppKit`-[NSButtonCell _sendActionFrom:] + 96
    frame #40: 0x00007fff338da32b AppKit`NSControlTrackMouse + 1745
    frame #41: 0x00007fff338d9c32 AppKit`-[NSCell trackMouse:inRect:ofView:untilMouseUp:] + 130
    frame #42: 0x00007fff338d9af1 AppKit`-[NSButtonCell trackMouse:inRect:ofView:untilMouseUp:] + 691
    frame #43: 0x00007fff338d8e6d AppKit`-[NSControl mouseDown:] + 748
    frame #44: 0x00007fff33a4c3ef AppKit`-[_NSThemeWidget mouseDown:] + 86
    frame #45: 0x00007fff338d7280 AppKit`-[NSWindow(NSEventRouting) _handleMouseDownEvent:isDelayedEvent:] + 4914
    frame #46: 0x00007fff33841a81 AppKit`-[NSWindow(NSEventRouting) _reallySendEvent:isDelayedEvent:] + 2612
    frame #47: 0x00007fff33840e29 AppKit`-[NSWindow(NSEventRouting) sendEvent:] + 349
    frame #48: 0x00007fff3383f1b4 AppKit`-[NSApplication(NSEvent) sendEvent:] + 352
    frame #49: 0x000000011bc15041 XUL`-[GeckoNSApplication sendEvent:](self=0x000000010ac4f660, _cmd="sendEvent:", anEvent=0x00000001113a2120) at nsAppShell.mm:138:3
    frame #50: 0x00007fff3368c21f AppKit`-[NSApplication run] + 707
    frame #51: 0x000000011bc1728e XUL`nsAppShell::Run(this=0x00000001101fd3a0) at nsAppShell.mm:690:5
    frame #52: 0x000000011ef633c0 XUL`nsAppStartup::Run(this=0x000000011037a2e0) at nsAppStartup.cpp:271:30
    frame #53: 0x000000011f179950 XUL`XREMain::XRE_mainRun(this=0x00007ffee54c41b8) at nsAppRunner.cpp:4690:22
    frame #54: 0x000000011f17aa0e XUL`XREMain::XRE_main(this=0x00007ffee54c41b8, argc=5, argv=0x00007ffee54c4840, aConfig=0x00007ffee54c4368) at nsAppRunner.cpp:4825:8
    frame #55: 0x000000011f17b0ab XUL`XRE_main(argc=5, argv=0x00007ffee54c4840, aConfig=0x00007ffee54c4368) at nsAppRunner.cpp:4879:21
    frame #56: 0x000000011f1963a7 XUL`mozilla::BootstrapImpl::XRE_main(this=0x000000010ac131e0, argc=5, argv=0x00007ffee54c4840, aConfig=0x00007ffee54c4368) at Bootstrap.cpp:45:12
    frame #57: 0x000000010a73c4be firefox`do_main(argc=5, argv=0x00007ffee54c4840, envp=0x00007ffee54c4870) at nsBrowserApp.cpp:217:22
    frame #58: 0x000000010a73bdb8 firefox`main(argc=5, argv=0x00007ffee54c4840, envp=0x00007ffee54c4870) at nsBrowserApp.cpp:331:16
    frame #59: 0x00007fff70365cc9 libdyld.dylib`start + 1

We are not set up to handle this. More specifically, what can happen is that we release mColorPanelWrapper[1] and then NULL out mCallback[2]. However, before mColorPanelWrapper is actually deallocated by the OS, the OS is sending us the second action, which results in a second call to nsColorPicker::Update[3]. One would be forgiven to think that it might be sufficient to add a NULL-check for mCallback in nsColorPicker::Update. However, this does not always work because mColorPanelWrapper's mColorPicker may already have been garbage collected after we called NS_RELEASE_THIS()[4]. In this case, mCallback is garbage by the time mColorPanelWrapper calls nsColorPicker::Update[3] the second time and we crash.

Finally, the reason why the crash signatures showed nsTSubstring<T>::StartBulkWriteImpl in the top stacks is simply due to optimisation and inlining. I am going to post two patches, the first of which is a minor refactor of our color picker code that also replaces our use of NSString's deprecated getCharacters: methods with a safe alternative, but I want to emphasize that this was not necessary for the actual fix here. I will describe the reasons for the refactor in Phabricator since it's not strictly necessary to fix the crash in this bug.

[1] https://searchfox.org/mozilla-central/rev/72e3388f74458d369af4f6cdbaeaacb719523b8c/widget/cocoa/nsColorPicker.mm#159
[2] https://searchfox.org/mozilla-central/rev/72e3388f74458d369af4f6cdbaeaacb719523b8c/widget/cocoa/nsColorPicker.mm#154
[3] https://searchfox.org/mozilla-central/rev/72e3388f74458d369af4f6cdbaeaacb719523b8c/widget/cocoa/nsColorPicker.mm#147
[4] https://searchfox.org/mozilla-central/rev/72e3388f74458d369af4f6cdbaeaacb719523b8c/widget/cocoa/nsColorPicker.mm#155

Comment 7

[Stephen A Pohl [:spohl]]

Attachment: Bug 1543813: Minor refactor of macOS color picker. r=mstange

Comment 8

[Stephen A Pohl [:spohl]]

Attachment: Bug 1543813: Fix a race condition that can lead to a crash when using the color picker on macOS. r=mstange

Depends on D71043

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c9b75701fcee
Minor refactor of macOS color picker. r=mstange
https://hg.mozilla.org/integration/autoland/rev/868982681ab3
Fix a race condition that can lead to a crash when using the color picker on macOS. r=mstange

Comment 11

[Razvan Maries]

Backed out for perma failures.

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&searchStr=os%2Cx%2C10.14%2Cdebug%2Cmochitests%2Ctest-macosx1014-64%2Fdebug-mochitest-browser-chrome-e10s-3%2Cm%28bc3%29&revision=0b74b615f7ad056f326e9e2147b21d18bc4bb89e&selectedTaskRun=DKY-vSw4RhOSkW9pCkIRcw-0

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=304025816&repo=autoland&lineNumber=2493

Backout: https://hg.mozilla.org/integration/autoland/rev/bf6e95a20230bc1bc33a32a47e59c5b3854d37b0

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ce05eb592dee
Minor refactor of macOS color picker. r=mstange
https://hg.mozilla.org/integration/autoland/rev/eacee6c149e7
Fix a race condition that can lead to a crash when using the color picker on macOS. r=mstange

Comment 13

[Razvan Maries]

Relanded: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=fcd578c812db156400ba68cfeb86d3d6474524bd

Sorry for the backout!

Comment 14

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/ce05eb592dee
https://hg.mozilla.org/mozilla-central/rev/eacee6c149e7