Bug 179792 – Fix occasional crash when adding and removing videos

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Fix occasional crash when adding and removing videos

[Alex Christensen]

Created attachment 327097 [details]
Patch

[Geoffrey Garen]

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?

[Alex Christensen]

(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

[Radar WebKit Bug Importer]

<rdar://problem/35608458>

[Alex Christensen]

rdar://problem/35485349

[Alex Christensen]

rdar://problem/28992186