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
Created attachment 394734 [details] seek-backward-support-crash-log
<rdar://problem/60976297>
I have marked this test as crashing while this issue is investigated. https://trac.webkit.org/changeset/259129/webkit
Created attachment 394955 [details] Patch
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.
> 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. >
(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.
*** Bug 209688 has been marked as a duplicate of this bug. ***
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
> > 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."
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.
> Please unmark tests. Will do.
> 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.
Created attachment 394972 [details] Patch for landing
Committed r259302: <https://trac.webkit.org/changeset/259302> All reviewed patches have been landed. Closing bug and clearing flags on attachment 394972 [details].