1491580 - Crash in -[ChildView initWithFrame:geckoChild:]

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Calixte Denizet (:calixte)]

This bug was filed from the Socorro interface and is
report bp-06333735-f132-4acc-94f7-140310180915.
=============================================================

Top 10 frames of crashing thread:

0 XUL -[ChildView initWithFrame:geckoChild:] widget/cocoa/nsChildView.mm:3375
1 XUL nsChildView::Create widget/cocoa/nsChildView.mm:526
2 XUL nsBaseWidget::CreateChild widget/nsBaseWidget.cpp:442
3 XUL nsView::CreateWidgetForParent view/nsView.cpp:616
4 XUL nsDocumentViewer::MakeWindow layout/base/nsDocumentViewer.cpp:2608
5 XUL nsDocumentViewer::InitInternal layout/base/nsDocumentViewer.cpp:983
6 XUL nsDocumentViewer::Init layout/base/nsDocumentViewer.cpp:771
7 XUL nsDocShell::Embed docshell/base/nsDocShell.cpp:8882
8 XUL nsDocShell::CreateAboutBlankContentViewer docshell/base/nsDocShell.cpp:7585
9 XUL nsAppShellService::JustCreateTopWindow xpfe/appshell/nsWebShellWindow.cpp:229

=============================================================

There are 27 crashes (from 3 installations) in nightly 64 with buildid 20180915101434. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 1486971.

[1] https://hg.mozilla.org/mozilla-central/rev?node=e92f3e990b35

Comment 1

[Markus Stange [:mstange]]

All these crashes occurred on 10.9. I'm not exactly sure which code is crashing here - is it just the access of the notification name (which is 10.10+), or is it the call to addObserver? Either way, wrapping the whole statement in a macOS version check should fix the crash.

This is pretty urgent to fix because it's a startup crash. It means that Firefox can't start at all on 10.9. Sorry for missing it in the review.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

That's totally my fault!  I thought the notification name is just a string, so it happens nothing on older versions.  So sorry for that.
 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5375d4a9d94c0a9b9daf51d8082123ff8e83b50

Hope this works.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1491580 - Use NSWorkspaceAccessibilityDisplayOptionsDidChangeNotification only for MacOSX 10.10 or later versions. r=mstange

Comment 4

[Hiroyuki Ikezoe (:hiro)]

I have to run right now, it's a kindergarten event today, will be back within 8 hours or so.

Comment 5

[Markus Stange [:mstange]]

Comment on attachment 9009412 [details]
Bug 1491580 - Use NSWorkspaceAccessibilityDisplayOptionsDidChangeNotification only for MacOSX 10.10 or later versions. r=mstange

Markus Stange [:mstange] has approved the revision.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/431d82d82470
Use NSWorkspaceAccessibilityDisplayOptionsDidChangeNotification only for MacOSX 10.10 or later versions. r=mstange

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Thank you Markus for reviewing and landing it!

Comment 8

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/431d82d82470

Comment 9

[Clément Lefèvre]

Verified as fixed: it was broken here and it's back to normal.

Comment 10

[Markus Stange [:mstange]]

Great. Thanks for fixing it so quickly Hiro!

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 9009412 [details]
Bug 1491580 - Use NSWorkspaceAccessibilityDisplayOptionsDidChangeNotification only for MacOSX 10.10 or later versions. r=mstange

Approval Request Comment
[Feature/Bug causing the regression]: bug 1486971 (but it's actually regressed only on nightly)
[User impact if declined]: Without this and with bug 1486971 firefox on MacOSX 10.9 crashes 
[Is this code covered by automated tests?]: A test case in bug 1486971 covers this but unfortunately we don't run the test on MacOSX 10.9. We are using MacOSX10.10 on our infra IIUC. 
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: I don't think we need manual test 
[List of other uplifts needed for the feature/fix]: This should be uplifted with bug 1486971.
[Is the change risky?]: Very low
[Why is the change risky/not risky?]: This just checks the MacOSX version, but it's really important for the crash.
[String changes made/needed]: None

Comment 12

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9009412 [details]
Bug 1491580 - Use NSWorkspaceAccessibilityDisplayOptionsDidChangeNotification only for MacOSX 10.10 or later versions. r=mstange

Uplift approved for 63 beta 8, thanks.

Comment 13

[Dorel Luca [:dluca]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9449c1f37ca7

Comment 14

[Marcia Knous [:marcia]]

This crash is still happening on 63 release, but apparently only to Mac users running 10.10: https://bit.ly/2ETsrgW. I can open a new bug if necessary.

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Marcia, yes, please open a new bug for that. We should definitely fix the crash.

Comment 16

[Marcia Knous [:marcia]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #15)
> Marcia, yes, please open a new bug for that. We should definitely fix the
> crash.

Filed Bug 1503424 for the 10.10 crashes.

Comment 17

[Hiroyuki Ikezoe (:hiro)]

Thank you, Marcia!