Bug 167766 – CrashTracer: [USER] com.apple.WebKit.WebContent at com.apple.WebCore: WebCore::URL::host const + 9

• Component: Media
• Status: RESOLVED FIXED

[Wenson Hsieh]

WebContent crash when vending a media session for the touch bar.

[Wenson Hsieh]

<rdar://problem/30132707>

[Wenson Hsieh]

Created attachment 300466 [details]
Patch

[Wenson Hsieh]

Created attachment 300469 [details]
Patch

[Chris Dumez]

Comment on attachment 300469 [details]
Patch

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

r=me with comment.

> Source/WebCore/html/HTMLMediaElement.cpp:586
> +    if (auto document = page.mainFrame().document()) {

We normally prefer earlier returns and auto*.

auto* document = page.mainFrame().document();
if (!document)
    return false;

String host = document->url().host();
return equalLettersIgnoringASCIICase(host, "netflix.com") || host.endsWithIgnoringASCIICase(".netflix.com");

[Wenson Hsieh]

Created attachment 300473 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 300473 [details]
Patch for landing

Rejecting attachment 300473 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 300473, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/2994162

[Alexey Proskuryakov]

Comment on attachment 300473 [details]
Patch for landing

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

> Source/WebCore/ChangeLog:10
> +        The mainframe's document pointer may be null when tearing down a page upon navigation to a page that is in the
> +        page cache. If this triggers a GC sweep, we will attempt to reload touch bar media controls, which (as a part of

Is the real bug here that work with side effects is being done during GC sweep? I don't think that it's an OK time for such work.

[Chris Dumez]

Comment on attachment 300473 [details]
Patch for landing

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

>> Source/WebCore/ChangeLog:10
>> +        page cache. If this triggers a GC sweep, we will attempt to reload touch bar media controls, which (as a part of
> 
> Is the real bug here that work with side effects is being done during GC sweep? I don't think that it's an OK time for such work.

Alexey is likely right that this work should not be done in the destructor but likely earlier (e.g. removed from document, ...).

That being said, I still think having a null check here makes sense as there is never a guarantee that a frame has a document. It is also a safe change if needed for a branch.
In addition to the null check though, we should find a better place to do this work than the destructor.

[Wenson Hsieh]

(In reply to comment #8)
> Comment on attachment 300473 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=300473&action=review
> 
> >> Source/WebCore/ChangeLog:10
> >> +        page cache. If this triggers a GC sweep, we will attempt to reload touch bar media controls, which (as a part of
> > 
> > Is the real bug here that work with side effects is being done during GC sweep? I don't think that it's an OK time for such work.
> 
> Alexey is likely right that this work should not be done in the destructor
> but likely earlier (e.g. removed from document, ...).
> 
> That being said, I still think having a null check here makes sense as there
> is never a guarantee that a frame has a document. It is also a safe change
> if needed for a branch.
> In addition to the null check though, we should find a better place to do
> this work than the destructor.

See also: <rdar://problem/30340495>

[Wenson Hsieh]

Committed r211613: <http://trac.webkit.org/changeset/211613>