Bug 209668 – REGRESSION: [ Mac wk2 Release ] Flaky crash in WebCore::MediaPlayer::createVideoFullscreenLayer

• Component: Media
• Status: RESOLVED FIXED

[Jason Lawrence]

media/modern-media-controls/seek-backward-support/seek-backward-support.html

Description:
This test is flaky crashing on Mac wk2 Release.
History:
https://results.webkit.org/?suite=layout-tests&test=media%2Fmodern-media-controls%2Fseek-backward-support%2Fseek-backward-support.html&style=release&flavor=wk2

Crash log;
Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x000000054324dd79 WebCore::MediaPlayer::createVideoFullscreenLayer() + 9 (MediaPlayer.cpp:762)
1   com.apple.WebCore             	0x0000000542de59a5 WebCore::HTMLMediaElement::createVideoFullscreenLayer() + 21 (HTMLMediaElement.cpp:6251)
2   com.apple.WebCore             	0x000000054253cf12 WebCore::VideoFullscreenModelVideoElement::createVideoFullscreenLayer() + 18 (VideoFullscreenModelVideoElement.mm:115)
3   com.apple.WebKit              	0x000000054045f03f WebKit::VideoFullscreenManager::enterVideoFullscreenForVideoElement(WebCore::HTMLVideoElement&, unsigned int, bool) + 387 (VideoFullscreenManager.mm:275)
4   libdispatch.dylib             	0x00007fff706376c4 _dispatch_call_block_and_release + 12
5   libdispatch.dylib             	0x00007fff70638658 _dispatch_client_callout + 8
6   libdispatch.dylib             	0x00007fff70643cab _dispatch_main_queue_callback_4CF + 936
7   com.apple.CoreFoundation      	0x00007fff36789041 __CFRUNLOOP_IS_SERVICING_THE_MAIN_DISPATCH_QUEUE__ + 9
8   com.apple.CoreFoundation      	0x00007fff36748e47 __CFRunLoopRun + 2028
9   com.apple.CoreFoundation      	0x00007fff36747ffe CFRunLoopRunSpecific + 462
10  com.apple.Foundation          	0x00007fff38ddc2a8 -[NSRunLoop(NSRunLoop) runMode:beforeDate:] + 212
11  com.apple.Foundation          	0x00007fff38e8ed2f -[NSRunLoop(NSRunLoop) run] + 76
12  libxpc.dylib                  	0x00007fff708df51a _xpc_objc_main.cold.4 + 49
13  libxpc.dylib                  	0x00007fff708df460 _xpc_objc_main + 559
14  libxpc.dylib                  	0x00007fff708def93 xpc_main + 377
15  com.apple.WebKit              	0x00000005401a48af WebKit::XPCServiceMain(int, char const**) + 554
16  libdyld.dylib                 	0x00007fff70691cc9 start + 1

[Jason Lawrence]

Created attachment 394734 [details]
seek-backward-support-crash-log

[Radar WebKit Bug Importer]

<rdar://problem/60976297>

[Jason Lawrence]

I have marked this test as crashing while this issue is investigated.
https://trac.webkit.org/changeset/259129/webkit

[Jer Noble]

Created attachment 394955 [details]
Patch

[Darin Adler]

Comment on attachment 394955 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=394955&action=review

> Source/WebCore/html/HTMLMediaElement.cpp:6246
> -    return m_player->createVideoFullscreenLayer();
> +    if (m_player)
> +        return m_player->createVideoFullscreenLayer();
> +    return nullptr;

Should be nil; this is an Objective-C class. Also would be good to do early return since that’s the WebKit "official" style.

Did you check that the callers can all handle nil?

> Source/WebCore/platform/cocoa/VideoFullscreenModelVideoElement.mm:117
> +    return nullptr;

Ditto.

[Jer Noble]

> View in context: https://bugs.webkit.org/attachment.cgi?id=394955&action=review
> 
> > Source/WebCore/html/HTMLMediaElement.cpp:6246
> > -    return m_player->createVideoFullscreenLayer();
> > +    if (m_player)
> > +        return m_player->createVideoFullscreenLayer();
> > +    return nullptr;
> 
> 
> Should be nil; this is an Objective-C class.

Pretty sure nil isn't a reserved word in .cpp files. Also, PlatformLayer is an Objective-C object in Cocoa ports, but other ports implement it differently, though this is inside a Cocoa-port-only #if guard.

> Also would be good to do early return since that’s the WebKit "official" style.

Is it really WebKit style to prefer (in a 3-line function):

if (!foo)
  return nullptr;
return foo->bar()

over:

if (foo)
  return foo->bar();
return nullptr;

> Did you check that the callers can all handle nil?

Yes; they all already have to handle that possibility since not all MediaPlayers can create a fullscreen layer.

> > Source/WebCore/platform/cocoa/VideoFullscreenModelVideoElement.mm:117
> > +    return nullptr;
> 
> 
> Ditto.
>

[Darin Adler]

(In reply to Jer Noble from comment #6)
> Pretty sure nil isn't a reserved word in .cpp files.

You can use nil in .cpp files, and should, for Objective-C object pointers. Whether it's a "reserved word" does not matter.

But nullptr would be fine if you prefer to use it.

> Is it really WebKit style to prefer (in a 3-line function):
> 
> if (!foo)
>   return nullptr;
> return foo->bar()
> 
> over:
> 
> if (foo)
>   return foo->bar();
> return nullptr;

Yes. But we could change it.

[Alexey Proskuryakov]

*** Bug 209688 has been marked as a duplicate of this bug. ***

[Alexey Proskuryakov]

Comment on attachment 394955 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=394955&action=review

> Source/WebCore/ChangeLog:7
> +        Reviewed by NOBODY (OOPS!).

Please unmark tests.

media/modern-media-controls/scrubber-support/scrubber-support-click.html
media/modern-media-controls/seek-backward-support/seek-backward-support.html

[Jer Noble]

> > Is it really WebKit style to prefer (in a 3-line function):
> > 
> > if (!foo)
> >   return nullptr;
> > return foo->bar()
> > 
> > over:
> > 
> > if (foo)
> >   return foo->bar();
> > return nullptr;
> 
> 
> Yes. But we could change it.

Huh. I went looking for this in the Coding Style Guidelines and couldn't find it. I know we prefer early returns over big if() statements, but I think this may be one of those unwritten "norms" that doesn't have an official entry in the Style Guidelines.

In my own internal stylebot, I have the rule as: "if returning early would make the function smaller, either vertically or horizontally, do so."

[Darin Adler]

The original early return rule was "return early for simple cases so that the main flow of the function isn't nested inside if statements". I suppose you could debate this point on a one line if statement body.

[Jer Noble]

> Please unmark tests.

Will do.

[Jer Noble]

> The original early return rule was "return early for simple cases so that the main flow of the function isn't nested inside if statements". I suppose you could debate this point on a one line if statement body.

For what it's worth, it looks like we're roughly split on whether we should do `if (!a) return nullptr; return a->b();` vs. `if (a) return a->b(); return nullptr;` with 667 instances of the first and 637 instances of the second inside WebKit.

[Jer Noble]

Created attachment 394972 [details]
Patch for landing

[EWS]

Committed r259302: <https://trac.webkit.org/changeset/259302>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 394972 [details].