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
Created attachment 253263 [details] proposed fix
Comment on attachment 253263 [details] 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.
Comment on attachment 253263 [details] proposed fix Clearing flags on attachment: 253263 Committed r184443: <http://trac.webkit.org/changeset/184443>
All reviewed patches have been landed. Closing bug.
> 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?
(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.
(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.