145083 2015-05-15 17:19:14 -0700 Crash when uploading huge files to YouTube or Google Drive 2024-04-12 14:26:05 -0700 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=272600 InRadar P2 Normal --- 1 ap ap commit-queue darin oldest_to_newest 1095310 0 ap 2015-05-15 17:19:14 -0700 Uploading a huge file to YouTube results in a crash, because FileReader is buggy. It's currently expected that uploading will fail, but we shouldn't crash. rdar://problem/15468529 1095394 1 253263 ap 2015-05-15 22:12:37 -0700 Created attachment 253263 proposed fix 1095474 2 253263 darin 2015-05-17 09:52:52 -0700 Comment on attachment 253263 proposed fix View in context: https://bugs.webkit.org/attachment.cgi?id=253263&action=review > Source/WebCore/fileapi/FileReaderLoader.cpp:196 > + newLength = std::max(newLength, m_totalBytes + m_totalBytes / 4 + 1); Where does this specific growth strategy (1.25x + 1) come from? The old code had doubling and I can imagine that was not good. But if I was writing the code I’m not sure I’d know to use this new strategy. Is there some theory behind it? Comment maybe? The old code tried to protect itself against overflow by computing the new length as a 64-bit value. This new version does not try to protect itself against overflow. Could that be a problem, or is there some reason that simply won’t happen? > Source/WebCore/fileapi/FileReaderLoader.cpp:197 > + RefPtr<ArrayBuffer> newData = ArrayBuffer::create(newLength, 1); The naming of ArrayBuffer::create is not good. If it’s a “try” type function, then it needs try in its name I think. Not this patch, but a design issue. 1095490 3 253263 commit-queue 2015-05-17 10:39:21 -0700 Comment on attachment 253263 proposed fix Clearing flags on attachment: 253263 Committed r184443: <http://trac.webkit.org/changeset/184443> 1095491 4 commit-queue 2015-05-17 10:39:26 -0700 All reviewed patches have been landed. Closing bug. 1095504 5 ap 2015-05-17 11:32:26 -0700 > Where does this specific growth strategy (1.25x + 1) come from? Just mimicking what WTF::Vector does. I do not know if there is any reason to not just use Vector, in fact. > The old code tried to protect itself against overflow by computing the new > length as a 64-bit value. This new version does not try to protect itself > against overflow. There is overflow protection in the new code, making sure that m_totalBytes + static_cast<unsigned>(dataLength) doesn't overflow. That's the bare minimum that we can continue with. If 1.25x + 1 overflows, that's not a problem - the result will be smaller, and we will just use the minimum (in the worst case, it could be a perf problem due to linear capacity growth). > newLength = std::max(newLength, m_totalBytes + m_totalBytes / 4 + 1); ^ newLength is already enough to append the new data. Does this sound right, or am I missing something? 1095616 6 darin 2015-05-18 08:10:02 -0700 (In reply to comment #5) > If 1.25x + 1 overflows, that's not a problem - the result will be smaller, > and we will just use the minimum (in the worst case, it could be a perf > problem due to linear capacity growth). OK. 1095617 7 darin 2015-05-18 08:10:56 -0700 (In reply to comment #5) > > Where does this specific growth strategy (1.25x + 1) come from? > > Just mimicking what WTF::Vector does. I do not know if there is any reason > to not just use Vector, in fact. Given that Vector's strategy might change in the future, we might want to do that thing where each code has s comment mentioning the other place. Using Vector directly is an interesting idea too. 253263 2015-05-15 22:12:37 -0700 2015-05-17 10:39:21 -0700 proposed fix UploadToYouTube.txt text/plain 4218 ap SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDE4NDQyMSkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDI1IEBACisyMDE1LTA1LTE1ICBBbGV4ZXkg UHJvc2t1cnlha292ICA8YXBAYXBwbGUuY29tPgorCisgICAgICAgIENyYXNoIHdoZW4gdXBsb2Fk aW5nIGh1Z2UgZmlsZXMgdG8gWW91VHViZSBvciBHb29nbGUgRHJpdmUKKyAgICAgICAgaHR0cHM6 Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE0NTA4MworICAgICAgICByZGFyOi8v cHJvYmxlbS8xNTQ2ODUyOQorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgor CisgICAgICAgIFRoaXMgZml4ZXMgdGhlIGNyYXNoLCBidXQgdXBsb2FkaW5nIHdpbGwgZmFpbC4K KworICAgICAgICAqIGZpbGVhcGkvRmlsZVJlYWRlckxvYWRlci5jcHA6CisgICAgICAgIChXZWJD b3JlOjpGaWxlUmVhZGVyTG9hZGVyOjpzdGFydCk6IFRlbGwgU3VicmVzb3VyY2VMb2FkZXIgdG8g bm90IHN0b3JlIGEgY29weSBvZgorICAgICAgICBhbGwgcmVjZWl2ZWQgZGF0YSwgRmlsZVJlYWRl ckxvYWRlciBoYXMgaXRzIG93biBidWZmZXIuCisgICAgICAgIChXZWJDb3JlOjpGaWxlUmVhZGVy TG9hZGVyOjpkaWRSZWNlaXZlUmVzcG9uc2UpOiBGaXhlZCBhIGJvdW5kcyBjaGVjayAtIG5vdCBl dmVyeQorICAgICAgICA2NC1iaXQgdmFsdWUgdGhhdCBkb2Vzbid0IGZpdCBpbnRvIDMyIGJpdHMg aXMgbmVnYXRpdmUuIFdpdGggdGhpcywgRmlsZVJlYWRlciBmYWlscworICAgICAgICBvbiBodWdl IGZpbGVzIHJpZ2h0IGF3YXksIGFzIGludGVuZGVkLgorICAgICAgICAoV2ViQ29yZTo6RmlsZVJl YWRlckxvYWRlcjo6ZGlkUmVjZWl2ZURhdGEpOiBGaXhlZCBtdWx0aXBsZSBidWdzIGluIGNvZGUg dGhhdCdzCisgICAgICAgIGV4ZWN1dGVkIHdoZW4gc2l6ZSBpcyBub3QgYXZhaWxhYmxlIHVwZnJv bnQuIFRoaXMgaXMgdGhlIGNvZGUgdGhhdCB1c2VkIHRvIGNyYXNoLAorICAgICAgICBidXQgd2l0 aCB0aGUgYWJvdmUgZml4LCBpdCdzIG5vdCBleGVjdXRlZCBieSBZb3VUdWJlLgorICAgICAgICBO b3Qgb25seSBvdmVyZmxvdyB3YXMgaGFuZGxlZCBpbmNvcnJlY3RseSwgYnV0IGV2ZW4gc2ltcGx5 IGdyb3dpbmcgYSBidWZmZXIgZm9yCisgICAgICAgIGFwcGVuZCB3YXMgYnVnZ3kuCisKIDIwMTUt MDUtMTUgIFNpbW9uIEZyYXNlciAgPHNpbW9uLmZyYXNlckBhcHBsZS5jb20+CiAKICAgICAgICAg UkVHUkVTU0lPTiAocjE4MzMwMCk6IEJhY2tncm91bmQgbWlzc2luZyBvbiB0b3AgbGlua3Mgb24g YXBwbGUuY29tCkluZGV4OiBTb3VyY2UvV2ViQ29yZS9maWxlYXBpL0ZpbGVSZWFkZXJMb2FkZXIu Y3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XZWJDb3JlL2ZpbGVhcGkvRmlsZVJlYWRlckxvYWRl ci5jcHAJKHJldmlzaW9uIDE4NDM5NCkKKysrIFNvdXJjZS9XZWJDb3JlL2ZpbGVhcGkvRmlsZVJl YWRlckxvYWRlci5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTk0LDYgKzk0LDcgQEAgdm9pZCBGaWxl UmVhZGVyTG9hZGVyOjpzdGFydChTY3JpcHRFeGVjdQogICAgIFRocmVhZGFibGVMb2FkZXJPcHRp b25zIG9wdGlvbnM7CiAgICAgb3B0aW9ucy5zZXRTZW5kTG9hZENhbGxiYWNrcyhTZW5kQ2FsbGJh Y2tzKTsKICAgICBvcHRpb25zLnNldFNuaWZmQ29udGVudChEb05vdFNuaWZmQ29udGVudCk7Cisg ICAgb3B0aW9ucy5zZXREYXRhQnVmZmVyaW5nUG9saWN5KERvTm90QnVmZmVyRGF0YSk7CiAgICAg b3B0aW9ucy5wcmVmbGlnaHRQb2xpY3kgPSBDb25zaWRlclByZWZsaWdodDsKICAgICBvcHRpb25z LnNldEFsbG93Q3JlZGVudGlhbHMoQWxsb3dTdG9yZWRDcmVkZW50aWFscyk7CiAgICAgb3B0aW9u cy5jcm9zc09yaWdpblJlcXVlc3RQb2xpY3kgPSBEZW55Q3Jvc3NPcmlnaW5SZXF1ZXN0czsKQEAg LTEzNiwxMSArMTM3LDEwIEBAIHZvaWQgRmlsZVJlYWRlckxvYWRlcjo6ZGlkUmVjZWl2ZVJlc3Bv bnMKICAgICAgICAgcmV0dXJuOwogICAgIH0KIAotICAgIHVuc2lnbmVkIGxvbmcgbG9uZyBsZW5n dGggPSByZXNwb25zZS5leHBlY3RlZENvbnRlbnRMZW5ndGgoKTsKKyAgICBsb25nIGxvbmcgbGVu Z3RoID0gcmVzcG9uc2UuZXhwZWN0ZWRDb250ZW50TGVuZ3RoKCk7CiAKLSAgICAvLyBBIHZhbHVl IGxhcmdlciB0aGFuIElOVF9NQVggbWVhbnMgdGhhdCB0aGUgY29udGVudCBsZW5ndGggd2Fzbid0 Ci0gICAgLy8gc3BlY2lmaWVkLCBzbyB0aGUgYnVmZmVyIHdpbGwgbmVlZCB0byBiZSBkeW5hbWlj YWxseSBncm93bi4KLSAgICBpZiAobGVuZ3RoID4gSU5UX01BWCkgeworICAgIC8vIEEgbmVnYXRp dmUgdmFsdWUgbWVhbnMgdGhhdCB0aGUgY29udGVudCBsZW5ndGggd2Fzbid0IHNwZWNpZmllZCwg c28gdGhlIGJ1ZmZlciB3aWxsIG5lZWQgdG8gYmUgZHluYW1pY2FsbHkgZ3Jvd24uCisgICAgaWYg KGxlbmd0aCA8IDApIHsKICAgICAgICAgbV92YXJpYWJsZUxlbmd0aCA9IHRydWU7CiAgICAgICAg IGlmIChtX2hhc1JhbmdlKQogICAgICAgICAgICAgbGVuZ3RoID0gMSArIG1fcmFuZ2VFbmQgLSBt X3JhbmdlU3RhcnQ7CkBAIC0xODgsMTcgKzE4OCwyNiBAQCB2b2lkIEZpbGVSZWFkZXJMb2FkZXI6 OmRpZFJlY2VpdmVEYXRhKGNvCiAgICAgICAgICAgICByZXR1cm47CiAgICAgICAgIH0KICAgICAg ICAgaWYgKG1fdmFyaWFibGVMZW5ndGgpIHsKLSAgICAgICAgICAgIHVuc2lnbmVkIGxvbmcgbG9u ZyBuZXdMZW5ndGggPSBtX3RvdGFsQnl0ZXMgKiAyOwotICAgICAgICAgICAgaWYgKG5ld0xlbmd0 aCA+IHN0ZDo6bnVtZXJpY19saW1pdHM8dW5zaWduZWQ+OjptYXgoKSkKLSAgICAgICAgICAgICAg ICBuZXdMZW5ndGggPSBzdGQ6Om51bWVyaWNfbGltaXRzPHVuc2lnbmVkPjo6bWF4KCk7Ci0gICAg ICAgICAgICBSZWZQdHI8QXJyYXlCdWZmZXI+IG5ld0RhdGEgPQotICAgICAgICAgICAgICAgIEFy cmF5QnVmZmVyOjpjcmVhdGUoc3RhdGljX2Nhc3Q8dW5zaWduZWQ+KG5ld0xlbmd0aCksIDEpOwor ICAgICAgICAgICAgdW5zaWduZWQgbmV3TGVuZ3RoID0gbV90b3RhbEJ5dGVzICsgc3RhdGljX2Nh c3Q8dW5zaWduZWQ+KGRhdGFMZW5ndGgpOworICAgICAgICAgICAgaWYgKG5ld0xlbmd0aCA8IG1f dG90YWxCeXRlcykgeworICAgICAgICAgICAgICAgIGZhaWxlZChGaWxlRXJyb3I6Ok5PVF9SRUFE QUJMRV9FUlIpOworICAgICAgICAgICAgICAgIHJldHVybjsKKyAgICAgICAgICAgIH0KKyAgICAg ICAgICAgIG5ld0xlbmd0aCA9IHN0ZDo6bWF4KG5ld0xlbmd0aCwgbV90b3RhbEJ5dGVzICsgbV90 b3RhbEJ5dGVzIC8gNCArIDEpOworICAgICAgICAgICAgUmVmUHRyPEFycmF5QnVmZmVyPiBuZXdE YXRhID0gQXJyYXlCdWZmZXI6OmNyZWF0ZShuZXdMZW5ndGgsIDEpOworICAgICAgICAgICAgaWYg KCFuZXdEYXRhKSB7CisgICAgICAgICAgICAgICAgLy8gTm90IGVub3VnaCBtZW1vcnkuCisgICAg ICAgICAgICAgICAgZmFpbGVkKEZpbGVFcnJvcjo6Tk9UX1JFQURBQkxFX0VSUik7CisgICAgICAg ICAgICAgICAgcmV0dXJuOworICAgICAgICAgICAgfQogICAgICAgICAgICAgbWVtY3B5KHN0YXRp Y19jYXN0PGNoYXIqPihuZXdEYXRhLT5kYXRhKCkpLCBzdGF0aWNfY2FzdDxjaGFyKj4obV9yYXdE YXRhLT5kYXRhKCkpLCBtX2J5dGVzTG9hZGVkKTsKIAogICAgICAgICAgICAgbV9yYXdEYXRhID0g bmV3RGF0YTsKICAgICAgICAgICAgIG1fdG90YWxCeXRlcyA9IHN0YXRpY19jYXN0PHVuc2lnbmVk PihuZXdMZW5ndGgpOwotICAgICAgICB9IGVsc2UKKyAgICAgICAgfSBlbHNlIHsKKyAgICAgICAg ICAgIC8vIFRoaXMgY2FuIG9ubHkgaGFwcGVuIGlmIHdlIGdldCBtb3JlIGRhdGEgdGhhbiBpbmRp Y2F0ZWQgaW4gZXhwZWN0ZWQgY29udGVudCBsZW5ndGggKGkuZS4gbmV2ZXIsIHVubGVzcyB0aGUg bmV0d29ya2luZyBsYXllciBpcyBidWdneSkuCiAgICAgICAgICAgICBsZW5ndGggPSByZW1haW5p bmdCdWZmZXJTcGFjZTsKKyAgICAgICAgfQogICAgIH0KIAogICAgIGlmIChsZW5ndGggPD0gMCkK