| Summary: | REGRESSION: [ Mac wk2 Release ] Flaky crash in WebCore::MediaPlayer::createVideoFullscreenLayer | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Jason Lawrence <Lawrence.j> | ||||||||
| Component: | Media | Assignee: | Jer Noble <jer.noble> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | calvaris, cdumez, darin, eric.carlson, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, jer.noble, peng.liu6, philipj, sergio, webkit-bot-watchers-bugzilla, webkit-bug-importer | ||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||
| Version: | WebKit Nightly Build | ||||||||||
| Hardware: | Mac | ||||||||||
| OS: | macOS 10.15 | ||||||||||
| See Also: |
https://bugs.webkit.org/show_bug.cgi?id=209752 https://bugs.webkit.org/show_bug.cgi?id=209688 |
||||||||||
| Attachments: |
|
||||||||||
|
Description
Jason Lawrence
2020-03-27 11:15:23 PDT
Created attachment 394734 [details]
seek-backward-support-crash-log
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]. |