194144 2019-01-31 22:49:27 -0800 Network Process crash when resuming downloads: '-[__NSDictionaryI setObject:forKey:]: unrecognized selector sent to instance %p' 2019-02-01 15:08:55 -0800 1 1 1 Unclassified WebKit WebKit2 WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 david_quesada webkit-unassigned achristensen cdumez commit-queue ggaren webkit-bug-importer oldest_to_newest 1501009 0 david_quesada 2019-01-31 22:49:27 -0800 rdar://problem/47553456 1501010 1 david_quesada 2019-01-31 22:54:59 -0800 Resuming a download occasionally causes a Network Process crash due to an uncaught NSInvalidArgumentException. In Download::resume(), we decode the root object from the resume data, assume it's a mutable dictionary (with no type checking), and try to -setObject:forKey: it. 1501135 2 360866 david_quesada 2019-02-01 09:39:16 -0800 Created attachment 360866 Patch 1501246 3 360866 ggaren 2019-02-01 13:26:55 -0800 Comment on attachment 360866 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=360866&action=review > Source/WebKit/NetworkProcess/Downloads/cocoa/DownloadCocoa.mm:61 > + auto dictionary = adoptNS(static_cast<NSMutableDictionary *>([[unarchiver decodeObjectOfClasses:plistClasses forKey:@"NSKeyedArchiveRootObjectKey"] mutableCopy])); Why do we pass plistClasses to decodeObjectOfClasses? My reading of this code is that any root object class other than NSDictionary would be an error. Should we just decodeObjectOfClass: [NSDictionary class]? 1501262 4 david_quesada 2019-02-01 13:45:45 -0800 (In reply to Geoffrey Garen from comment #3) > Comment on attachment 360866 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=360866&action=review > > > Source/WebKit/NetworkProcess/Downloads/cocoa/DownloadCocoa.mm:61 > > + auto dictionary = adoptNS(static_cast<NSMutableDictionary *>([[unarchiver decodeObjectOfClasses:plistClasses forKey:@"NSKeyedArchiveRootObjectKey"] mutableCopy])); > > Why do we pass plistClasses to decodeObjectOfClasses? My reading of this > code is that any root object class other than NSDictionary would be an error. > > Should we just decodeObjectOfClass: [NSDictionary class]? That won't work. The class whitelist also applies to the objects being decoded by the dictionary. So if we only allow decoding NSDictionary, we wouldn't get any object unless the resume data is a dictionary that only contains other dictionaries as objects (which themselves can only contain dictionaries as objects, recursively). 1501292 5 360866 ggaren 2019-02-01 14:31:21 -0800 Comment on attachment 360866 Patch r=me 1501307 6 360866 commit-queue 2019-02-01 15:08:53 -0800 Comment on attachment 360866 Patch Clearing flags on attachment: 360866 Committed r240881: <https://trac.webkit.org/changeset/240881> 1501308 7 commit-queue 2019-02-01 15:08:55 -0800 All reviewed patches have been landed. Closing bug. 360866 2019-02-01 09:39:16 -0800 2019-02-01 15:08:53 -0800 Patch file_194144.txt text/plain 2685 david_quesada Y29tbWl0IDg5YjdmZTUxYTQwZWJjM2E5YzA5MzRhYmVlYjBlZTg5NGE2YzBlYzcNCkF1dGhvcjog RGF2aWQgUXVlc2FkYSA8ZGF2aWRfcXVlc2FkYUBhcHBsZS5jb20+DQpEYXRlOiAgIEZyaSBGZWIg MSAwOTozNzo0NSAyMDE5IC0wODAwDQoNCiAgICBOZXR3b3JrIFByb2Nlc3MgY3Jhc2ggd2hlbiBy ZXN1bWluZyBkb3dubG9hZHM6ICctW19fTlNEaWN0aW9uYXJ5SSBzZXRPYmplY3Q6Zm9yS2V5Ol06 IHVucmVjb2duaXplZCBzZWxlY3RvciBzZW50IHRvIGluc3RhbmNlICVwJw0KICAgIGh0dHBzOi8v YnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xOTQxNDQNCiAgICByZGFyOi8vcHJvYmxl bS80NzU1MzQ1Ng0KICAgIA0KICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLg0KICAgIA0K ICAgICogTmV0d29ya1Byb2Nlc3MvRG93bmxvYWRzL2NvY29hL0Rvd25sb2FkQ29jb2EubW06DQog ICAgKFdlYktpdDo6RG93bmxvYWQ6OnJlc3VtZSk6DQogICAgICAgIE1ha2UgYSBtdXRhYmxlIGNv cHkgb2YgdGhlIHJvb3Qgb2JqZWN0IGRlY29kZWQgZnJvbSB0aGUgcmVzdW1lIGRhdGEuDQogICAg ICAgIEl0IG1pZ2h0IGhhdmUgYmVlbiBvcmlnaW5hbGx5IGVuY29kZWQgYXMgYW4gaW1tdXRhYmxl IGRpY3Rpb25hcnkuDQoNCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L0NoYW5nZUxvZyBiL1Nv dXJjZS9XZWJLaXQvQ2hhbmdlTG9nDQppbmRleCAyYjZiNWYzMDE2Ny4uNzNmMTI4YzU0NzkgMTAw NjQ0DQotLS0gYS9Tb3VyY2UvV2ViS2l0L0NoYW5nZUxvZw0KKysrIGIvU291cmNlL1dlYktpdC9D aGFuZ2VMb2cNCkBAIC0xLDMgKzEsMTYgQEANCisyMDE5LTAyLTAxICBEYXZpZCBRdWVzYWRhICA8 ZGF2aWRfcXVlc2FkYUBhcHBsZS5jb20+DQorDQorICAgICAgICBOZXR3b3JrIFByb2Nlc3MgY3Jh c2ggd2hlbiByZXN1bWluZyBkb3dubG9hZHM6ICctW19fTlNEaWN0aW9uYXJ5SSBzZXRPYmplY3Q6 Zm9yS2V5Ol06IHVucmVjb2duaXplZCBzZWxlY3RvciBzZW50IHRvIGluc3RhbmNlICVwJw0KKyAg ICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE5NDE0NA0KKyAg ICAgICAgcmRhcjovL3Byb2JsZW0vNDc1NTM0NTYNCisNCisgICAgICAgIFJldmlld2VkIGJ5IE5P Qk9EWSAoT09QUyEpLg0KKw0KKyAgICAgICAgKiBOZXR3b3JrUHJvY2Vzcy9Eb3dubG9hZHMvY29j b2EvRG93bmxvYWRDb2NvYS5tbToNCisgICAgICAgIChXZWJLaXQ6OkRvd25sb2FkOjpyZXN1bWUp Og0KKyAgICAgICAgICAgIE1ha2UgYSBtdXRhYmxlIGNvcHkgb2YgdGhlIHJvb3Qgb2JqZWN0IGRl Y29kZWQgZnJvbSB0aGUgcmVzdW1lIGRhdGEuDQorICAgICAgICAgICAgSXQgbWlnaHQgaGF2ZSBi ZWVuIG9yaWdpbmFsbHkgZW5jb2RlZCBhcyBhbiBpbW11dGFibGUgZGljdGlvbmFyeS4NCisNCiAy MDE5LTAxLTMxICBUYWthc2hpIEtvbW9yaSAgPFRha2FzaGkuS29tb3JpQHNvbnkuY29tPg0KIA0K ICAgICAgICAgW0N1cmxdIFJlbW92ZSB1bm5lY2Vzc2FyeSBtZW1iZXIgZnJvbSBOZXR3b3JrU3Rv cmFnZVNlc3Npb24uDQpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYktpdC9OZXR3b3JrUHJvY2Vzcy9E b3dubG9hZHMvY29jb2EvRG93bmxvYWRDb2NvYS5tbSBiL1NvdXJjZS9XZWJLaXQvTmV0d29ya1By b2Nlc3MvRG93bmxvYWRzL2NvY29hL0Rvd25sb2FkQ29jb2EubW0NCmluZGV4IDFiOWUzZjIyN2Rm Li5kNmQ4YzlhNDVjZCAxMDA2NDQNCi0tLSBhL1NvdXJjZS9XZWJLaXQvTmV0d29ya1Byb2Nlc3Mv RG93bmxvYWRzL2NvY29hL0Rvd25sb2FkQ29jb2EubW0NCisrKyBiL1NvdXJjZS9XZWJLaXQvTmV0 d29ya1Byb2Nlc3MvRG93bmxvYWRzL2NvY29hL0Rvd25sb2FkQ29jb2EubW0NCkBAIC01OCw3ICs1 OCw3IEBAIHZvaWQgRG93bmxvYWQ6OnJlc3VtZShjb25zdCBJUEM6OkRhdGFSZWZlcmVuY2UmIHJl c3VtZURhdGEsIGNvbnN0IFN0cmluZyYgcGF0aCwNCiAgICAgfSk7DQogICAgIGF1dG8gdW5hcmNo aXZlciA9IGFkb3B0TlMoW1tOU0tleWVkVW5hcmNoaXZlciBhbGxvY10gaW5pdEZvclJlYWRpbmdG cm9tRGF0YTpuc0RhdGEuZ2V0KCkgZXJyb3I6bmlsXSk7DQogICAgIFt1bmFyY2hpdmVyIHNldERl Y29kaW5nRmFpbHVyZVBvbGljeTpOU0RlY29kaW5nRmFpbHVyZVBvbGljeVJhaXNlRXhjZXB0aW9u XTsNCi0gICAgYXV0byBkaWN0aW9uYXJ5ID0gcmV0YWluUHRyKFt1bmFyY2hpdmVyIGRlY29kZU9i amVjdE9mQ2xhc3NlczpwbGlzdENsYXNzZXMgZm9yS2V5OkAiTlNLZXllZEFyY2hpdmVSb290T2Jq ZWN0S2V5Il0pOw0KKyAgICBhdXRvIGRpY3Rpb25hcnkgPSBhZG9wdE5TKHN0YXRpY19jYXN0PE5T TXV0YWJsZURpY3Rpb25hcnkgKj4oW1t1bmFyY2hpdmVyIGRlY29kZU9iamVjdE9mQ2xhc3Nlczpw bGlzdENsYXNzZXMgZm9yS2V5OkAiTlNLZXllZEFyY2hpdmVSb290T2JqZWN0S2V5Il0gbXV0YWJs ZUNvcHldKSk7DQogICAgIFt1bmFyY2hpdmVyIGZpbmlzaERlY29kaW5nXTsNCiAgICAgW2RpY3Rp b25hcnkgc2V0T2JqZWN0OnN0YXRpY19jYXN0PE5TU3RyaW5nKj4ocGF0aCkgZm9yS2V5OkAiTlNV UkxTZXNzaW9uUmVzdW1lSW5mb0xvY2FsUGF0aCJdOw0KICAgICBhdXRvIGVuY29kZXIgPSBhZG9w dE5TKFtbTlNLZXllZEFyY2hpdmVyIGFsbG9jXSBpbml0UmVxdWlyaW5nU2VjdXJlQ29kaW5nOllF U10pOw0K