Bug 220776

Summary: Fix nullptr dereference introduced in r268228
Product: WebKit Reporter: Alex Christensen <achristensen>
Component: New BugsAssignee: Alex Christensen <achristensen>
Status: RESOLVED FIXED    
Severity: Normal CC: bugmail, cdumez, feross, jhiesey, lwarlow, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch ews-feeder: commit-queue-

Description Alex Christensen 2021-01-20 11:34:39 PST
Fix nullptr dereference introduced in r268228
Comment 1 Alex Christensen 2021-01-20 11:36:50 PST
Created attachment 417983 [details]
Patch
Comment 2 Chris Dumez 2021-01-20 11:40:49 PST
Comment on attachment 417983 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=417983&action=review

> Source/WebCore/fileapi/Blob.cpp:274
> +            if (bytesLoaded == m_loader->totalBytes()&& !m_bytesRead)

nit: missing space before &&
Comment 3 Chris Dumez 2021-01-20 11:42:11 PST
Comment on attachment 417983 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=417983&action=review

>> Source/WebCore/fileapi/Blob.cpp:274
>> +            if (bytesLoaded == m_loader->totalBytes()&& !m_bytesRead)
> 
> nit: missing space before &&

Also may want to use FileReaderLoader::isCompleted() for clarity.
Comment 4 Alex Christensen 2021-01-20 11:44:33 PST
Created attachment 417984 [details]
Patch
Comment 5 EWS 2021-01-20 13:10:00 PST
Committed r271669: <https://trac.webkit.org/changeset/271669>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 417984 [details].
Comment 6 Radar WebKit Bug Importer 2021-01-20 13:10:15 PST
<rdar://problem/73417187>
Comment 7 Alex Christensen 2021-01-20 17:26:48 PST
This was rdar://problem/71627170
Comment 8 John Hiesey 2021-04-26 19:08:32 PDT
It is unfortunate that Safari 14.1 shipped with Blob.stream() but without this fix.

Our web app, wormhole.app, uses feature detection to determine if Blob.stream() is available, so it broke when Safari 14.1 was released. We had to quickly add a patch to disable using Blob.stream() in Safari, since it would immediately crash the renderer process.

If the feature was known to be buggy, I wish it hadn't shipped, or that the fix had been applied before shipping it.

We did test in Safari Technology Preview, but it already had this fix applied so we didn't encounter the crash.
Comment 9 Chris Dumez 2021-04-27 11:55:29 PDT
(In reply to John Hiesey from comment #8)
> It is unfortunate that Safari 14.1 shipped with Blob.stream() but without
> this fix.
> 
> Our web app, wormhole.app, uses feature detection to determine if
> Blob.stream() is available, so it broke when Safari 14.1 was released. We
> had to quickly add a patch to disable using Blob.stream() in Safari, since
> it would immediately crash the renderer process.
> 
> If the feature was known to be buggy, I wish it hadn't shipped, or that the
> fix had been applied before shipping it.
> 
> We did test in Safari Technology Preview, but it already had this fix
> applied so we didn't encounter the crash.

Thanks for bringing this to our attention. We'll look into what happened.