1607140 - Crash in [@ nsTouchBarInputIcon::SetupIcon]

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Gabriele Svelto [:gsvelto] (PTO)]

This bug is for crash report bp-8a60b34b-6ce1-4890-9661-6c9fd0200106.

Top 10 frames of crashing thread:

0 XUL nsTouchBarInputIcon::SetupIcon widget/cocoa/nsTouchBarInputIcon.mm:67
1 XUL -[nsTouchBar loadIconForInput:forItem:] widget/cocoa/nsTouchBar.mm:534
2 XUL -[nsTouchBar updatePopover:withIdentifier:] widget/cocoa/nsTouchBar.mm:368
3 XUL -[nsTouchBar touchBar:makeItemForIdentifier:] widget/cocoa/nsTouchBar.mm
4 AppKit __32-[NSTouchBar itemForIdentifier:]_block_invoke 
5 AppKit +[NSAppearance _performWithCurrentAppearance:usingBlock:] 
6 AppKit -[NSTouchBar itemForIdentifier:] 
7 XUL -[nsTouchBar releaseJSObjects] widget/cocoa/nsTouchBar.mm:550
8 XUL nsCocoaWindow::DestroyNativeWindow widget/cocoa/nsCocoaWindow.mm:173
9 XUL nsCocoaWindow::~nsCocoaWindow widget/cocoa/nsCocoaWindow.mm:207

This looks like a dereference of a NULL this pointer.

Comment 1

[Anne (:annevk)]

I hit this as well (https://crash-stats.mozilla.org/report/index/c92779fd-56fe-4773-9056-2eb760200106) and my MacBook is closed as I use an external monitor and keyboard. Thought that might be worth mentioning.

Comment 2

[Marcia Knous [:marcia]]

Bug 1581555 is in the regression range. ni on harry

Comment 3

[Harry Twyford]

Marcia, do you have a full regression range? I landed two patches that touched nsTouchBarInputIcon within an hour or so of each other. Bug 1581555 and bug 1600356. Is the latter bug in the range?

Comment 4

[Marcia Knous [:marcia]]

Crashes started in 20200104094647. Full regression range: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c0a6eb95b65cce34497c3f6aca502934ed0191cc&tochange=38e6ad5fd7535be88e432075f76ec4a2dc294672

From the crash stack it looks as if Bug 1600356 might have touched code.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Bug 1600356 looks like a pretty big set of changes to land during a soft freeze :(. I'm thinking we should back it out from Beta ASAP.

Comment 6

[Harry Twyford]

Works for me. I asked Ryan via PM that Bug 1581555 be backed out from m-c as well, so I can determine which of the two patches is causing this crash. Leaving ni=me since I'll continue to investigate.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Both bugs from comment 3 have been backed out from mozilla-beta, so 73.0b1 shouldn't be affected by these crashes. Also, per comment 6, bug 1581555 was backed out from m-c to see what affect that has on the Nightly crash rate.

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug is a regression, but please revert this change in case of error.

Comment 9

[Christoph Walcher]

I hit this crash several times on my MBP with TB too.
Maybe the check on the call site of SetupIcon isn't sufficent https://searchfox.org/mozilla-central/source/widget/cocoa/nsTouchBar.mm#525
This change was added in bug1600356
Adding

if (!mTouchBarHelper) {
   return;
}

could help

Comment 10

[Harry Twyford]

Ah, thank you Christoph. I have a feeling that's it. I'll prepare a patch.

Comment 11

[Harry Twyford]

Attachment: Bug 1607140 - Add a more stringent check for mTouchBarHelper in loadIconForInput. r?spohl!

Comment 12

[Christoph Walcher]

While looking through the stacktrace I noticed a new touchbar is instantiated during releaseJSObjects, because the call to itemForIdentifier implicitly calls makeItemForIdentifier if the item isn't yet created. To prevent this a early return in makeItemForIdentifiercould save some resources. e.g.:

if (!mTouchBarHelper) {
   return nil;
}

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/320f70246faf
Add a more stringent check for mTouchBarHelper in loadIconForInput. r=spohl

Comment 14

[Stephen A Pohl [:spohl]]

See comment 12

Comment 15

[Harry Twyford]

Attachment: Bug 1607140 - Add mTouchBarHelper null-checks to makeItemForIdentifier and updateItem. r?spohl

Comment 16

[Harry Twyford]

Thanks for the needinfo. I missed comment 12 before pushing.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ae6bb3a10669
Add mTouchBarHelper null-checks to makeItemForIdentifier and updateItem. r=spohl

Comment 18

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/320f70246faf
https://hg.mozilla.org/mozilla-central/rev/ae6bb3a10669

Comment 19

[Harry Twyford]

Looks like the fix resolved this, so I'm going to re-land the backed out patches into 74.