Bug 179792 - Fix occasional crash when adding and removing videos
Summary: Fix occasional crash when adding and removing videos
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-11-16 13:03 PST by Alex Christensen
Modified: 2017-11-27 15:39 PST (History)
2 users (show)

See Also:


Attachments
Patch (1.47 KB, patch)
2017-11-16 13:06 PST, Alex Christensen
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2017-11-16 13:03:46 PST
Fix occasional crash when adding and removing videos
Comment 1 Alex Christensen 2017-11-16 13:06:16 PST
Created attachment 327097 [details]
Patch
Comment 2 Geoffrey Garen 2017-11-16 16:48:31 PST
Comment on attachment 327097 [details]
Patch

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

r=me

> Source/WebCore/ChangeLog:11
> +        Right now we are calling a method on self.session.loader which is a PlatformMediaResourceLoader&
> +        but in ObjC if session is null it will call a method on null, which crashes.

I don't understand this explanation. A message to nil is well-defined in Objective-C: it returns a zero value and does not crash. 

Maybe the actual problem is that _restart calls the C++ member function requestResource, passing nullptr as the |this| value. Is the the case?

Would be good to fix this explanation.

> Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:451
> +    if (!self.session)
> +        return;
> +
>      ASSERT(isMainThread());
>      [self _cancel];

Should we put the ASSERT at the top so we don't skip it in the null session case?
Comment 3 Alex Christensen 2017-11-16 21:50:51 PST
(In reply to Geoffrey Garen from comment #2)
> I don't understand this explanation. A message to nil is well-defined in
> Objective-C: it returns a zero value and does not crash. 
> 
> Maybe the actual problem is that _restart calls the C++ member function
> requestResource, passing nullptr as the |this| value. Is the the case?
That's exactly what's going on.

> Would be good to fix this explanation.
I did.

> > Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:451
> > +    if (!self.session)
> > +        return;
> > +
> >      ASSERT(isMainThread());
> >      [self _cancel];
> 
> Should we put the ASSERT at the top so we don't skip it in the null session
> case?
Good idea!  Done.

http://trac.webkit.org/r224960
Comment 4 Radar WebKit Bug Importer 2017-11-16 21:51:16 PST
<rdar://problem/35608458>
Comment 5 Alex Christensen 2017-11-17 13:52:37 PST
rdar://problem/35485349
Comment 6 Alex Christensen 2017-11-27 15:39:34 PST
rdar://problem/28992186