Bug 145083 – Crash when uploading huge files to YouTube or Google Drive

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Alexey Proskuryakov]

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

[Alexey Proskuryakov]

Created attachment 253263 [details]
proposed fix

[Darin Adler]

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.

[WebKit Commit Bot]

Comment on attachment 253263 [details]
proposed fix

Clearing flags on attachment: 253263

Committed r184443: <http://trac.webkit.org/changeset/184443>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

> 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?

[Darin Adler]

(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.

[Darin Adler]

(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.