179792 2017-11-16 13:03:46 -0800 Fix occasional crash when adding and removing videos 2017-11-27 15:39:34 -0800 1 1 1 Unclassified WebKit New Bugs WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 achristensen achristensen ggaren webkit-bug-importer oldest_to_newest 1372770 0 achristensen 2017-11-16 13:03:46 -0800 Fix occasional crash when adding and removing videos 1372771 1 327097 achristensen 2017-11-16 13:06:16 -0800 Created attachment 327097 Patch 1372891 2 327097 ggaren 2017-11-16 16:48:31 -0800 Comment on attachment 327097 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? 1372976 3 achristensen 2017-11-16 21:50:51 -0800 (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 1372977 4 webkit-bug-importer 2017-11-16 21:51:16 -0800 <rdar://problem/35608458> 1373270 5 achristensen 2017-11-17 13:52:37 -0800 rdar://problem/35485349 1375182 6 achristensen 2017-11-27 15:39:34 -0800 rdar://problem/28992186 327097 2017-11-16 13:06:16 -0800 2017-11-16 16:48:31 -0800 Patch bug-179792-20171116130615.patch text/plain 1509 achristensen SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDIyNDkzNCkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE5IEBACisyMDE3LTExLTE2ICBBbGV4IENo cmlzdGVuc2VuICA8YWNocmlzdGVuc2VuQHdlYmtpdC5vcmc+CisKKyAgICAgICAgRml4IG9jY2Fz aW9uYWwgY3Jhc2ggd2hlbiBhZGRpbmcgYW5kIHJlbW92aW5nIHZpZGVvcworICAgICAgICBodHRw czovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTc5NzkyCisKKyAgICAgICAgUmV2 aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgSW4gdGFza0NvbXBsZXRlZCB3ZSBu dWxsIG91dCBzZXNzaW9uLCBhbmQgaW4gcmVzdW1lIHdlIGNhbGwgX3Jlc3RhcnQgb24gdGhlIG1h aW4gdGhyZWFkLgorICAgICAgICBJZiBfcmVzdGFydCBpcyBjYWxsZWQgYWZ0ZXIgdGFza0NvbXBs ZXRlZCwgd2Ugd2FudCB0byBkbyBub3RoaW5nLgorICAgICAgICBSaWdodCBub3cgd2UgYXJlIGNh bGxpbmcgYSBtZXRob2Qgb24gc2VsZi5zZXNzaW9uLmxvYWRlciB3aGljaCBpcyBhIFBsYXRmb3Jt TWVkaWFSZXNvdXJjZUxvYWRlciYKKyAgICAgICAgYnV0IGluIE9iakMgaWYgc2Vzc2lvbiBpcyBu dWxsIGl0IHdpbGwgY2FsbCBhIG1ldGhvZCBvbiBudWxsLCB3aGljaCBjcmFzaGVzLgorCisgICAg ICAgICogcGxhdGZvcm0vbmV0d29yay9jb2NvYS9XZWJDb3JlTlNVUkxTZXNzaW9uLm1tOgorICAg ICAgICAoLVtXZWJDb3JlTlNVUkxTZXNzaW9uRGF0YVRhc2sgX3Jlc3RhcnRdKToKKyAgICAgICAg RWFybHkgcmV0dXJuIGlmIHdlIGRvbid0IGhhdmUgYSBzZXNzaW9uLgorCiAyMDE3LTExLTE2ICBB bnRvaW5lIFF1aW50ICA8Z3Jhb3V0c0BhcHBsZS5jb20+CiAKICAgICAgICAgQ2xlYW4gdXAgS2V5 ZnJhbWVFZmZlY3QKSW5kZXg6IFNvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL25ldHdvcmsvY29jb2Ev V2ViQ29yZU5TVVJMU2Vzc2lvbi5tbQo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2ViQ29yZS9wbGF0 Zm9ybS9uZXR3b3JrL2NvY29hL1dlYkNvcmVOU1VSTFNlc3Npb24ubW0JKHJldmlzaW9uIDIyNDg5 NSkKKysrIFNvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL25ldHdvcmsvY29jb2EvV2ViQ29yZU5TVVJM U2Vzc2lvbi5tbQkod29ya2luZyBjb3B5KQpAQCAtNDQ0LDYgKzQ0NCw5IEBAIC0gKGlkKWNvcHlX aXRoWm9uZTooTlNab25lICopem9uZQogCiAtICh2b2lkKV9yZXN0YXJ0CiB7CisgICAgaWYgKCFz ZWxmLnNlc3Npb24pCisgICAgICAgIHJldHVybjsKKwogICAgIEFTU0VSVChpc01haW5UaHJlYWQo KSk7CiAgICAgW3NlbGYgX2NhbmNlbF07CiAK