Bug 233923

Summary: Make PreviewConverterProvider not modify the SharedBuffer once returned
Product: WebKit Reporter: Jean-Yves Avenard [:jya] <jean-yves.avenard>
Component: WebCore Misc.Assignee: Jean-Yves Avenard [:jya] <jean-yves.avenard>
Status: RESOLVED FIXED    
Severity: Normal CC: alecflett, beidson, benjamin, calvaris, cdumez, cgarcia, changseok, eric.carlson, esprehn+autocc, ews-watchlist, galpeter, glenn, gustavo, gyuyoung.kim, hi, hta, japhet, jbedard, jer.noble, joepeck, jsbell, kangil.han, keith_miller, macpherson, mark.lam, menard, mifenton, mmaxfield, msaboff, pangle, philipj, pnormand, rniwa, saam, sergio, tommyw, tzagallo, vjaquez, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 233922    
Attachments:
Description Flags
Patch for EWS
none
Patch
none
Patch
none
Patch for EWS
none
Patch
none
Patch for EWS
none
Patch none

Description Jean-Yves Avenard [:jya] 2021-12-07 04:07:41 PST 
The LegacyPreviewLoader keeps a SharedBuffer, add data to it and pass it to the provideMainResourceForPreviewConverter PreviewConverter and then clear the Sharedbuffer

The original object passed to provideMainResourceForPreviewConverter is now empty.

In practice here is doesn't matter as the PreviewConverter will read the content immediately, however refactoring this code will allow to have the SharedBufferBuilder::get() method always return a new object.
Comment 1 Radar WebKit Bug Importer 2021-12-07 04:09:47 PST 
<rdar://problem/86149850>
Comment 2 Jean-Yves Avenard [:jya] 2021-12-07 04:18:11 PST 
Created attachment 446155 [details]
Patch for EWS
Comment 3 Jean-Yves Avenard [:jya] 2021-12-07 04:19:01 PST 
Created attachment 446156 [details]
Patch
Comment 4 Jean-Yves Avenard [:jya] 2021-12-07 17:34:26 PST 
Created attachment 446266 [details]
Patch
Comment 5 Jean-Yves Avenard [:jya] 2021-12-07 17:35:50 PST 
Created attachment 446267 [details]
Patch for EWS
Comment 6 Jean-Yves Avenard [:jya] 2021-12-09 06:36:38 PST 
Created attachment 446537 [details]
Patch

rebase
Comment 7 Jean-Yves Avenard [:jya] 2021-12-09 06:37:42 PST 
Created attachment 446538 [details]
Patch for EWS
Comment 8 Jean-Yves Avenard [:jya] 2021-12-14 03:30:09 PST 
Created attachment 447121 [details]
Patch

rebase
Comment 9 youenn fablet 2021-12-14 05:37:46 PST 
Comment on attachment 447121 [details]
Patch

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

> Source/WebCore/loader/ios/LegacyPreviewLoader.mm:230
> +    completionHandler(m_originalData.copy());

Unrelated but why do we need a CompletionHandler if we are synchronously returning.

> Source/WebCore/platform/PreviewConverter.cpp:85
> +        appendFromBuffer(WTFMove(buffer));

So we can never go to didFailUpdating, even if buffer is of size 0?
Should we move from const FragmentedSharedBuffer* to RefPtr<FragmentedSharedBuffer>&&?
Comment 10 Jean-Yves Avenard [:jya] 2021-12-14 13:15:36 PST 
Comment on attachment 447121 [details]
Patch

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

>> Source/WebCore/platform/PreviewConverter.cpp:85
>> +        appendFromBuffer(WTFMove(buffer));
> 
> So we can never go to didFailUpdating, even if buffer is of size 0?
> Should we move from const FragmentedSharedBuffer* to RefPtr<FragmentedSharedBuffer>&&?

Buffer previously was a pointer and could never have been null (the SharedBuffer was always created in the constructor)
I made it a Ref<FragmentedaharedBuffer>&&

There’s been no logic change
Comment 11 EWS 2021-12-14 15:06:06 PST 
Committed r287053 (245249@main): <https://commits.webkit.org/245249@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 447121 [details].