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

Jean-Yves Avenard [:jya]
Reported 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.
Attachments
Patch for EWS (737.62 KB, patch)
2021-12-07 04:18 PST, Jean-Yves Avenard [:jya]
no flags
Patch (5.54 KB, patch)
2021-12-07 04:19 PST, Jean-Yves Avenard [:jya]
no flags
Patch (5.58 KB, patch)
2021-12-07 17:34 PST, Jean-Yves Avenard [:jya]
no flags
Patch for EWS (737.66 KB, patch)
2021-12-07 17:35 PST, Jean-Yves Avenard [:jya]
no flags
Patch (5.48 KB, patch)
2021-12-09 06:36 PST, Jean-Yves Avenard [:jya]
no flags
Patch for EWS (736.71 KB, patch)
2021-12-09 06:37 PST, Jean-Yves Avenard [:jya]
no flags
Patch (5.48 KB, patch)
2021-12-14 03:30 PST, Jean-Yves Avenard [:jya]
no flags
Radar WebKit Bug Importer
Comment 1 2021-12-07 04:09:47 PST
Jean-Yves Avenard [:jya]
Comment 2 2021-12-07 04:18:11 PST
Created attachment 446155 [details] Patch for EWS
Jean-Yves Avenard [:jya]
Comment 3 2021-12-07 04:19:01 PST
Jean-Yves Avenard [:jya]
Comment 4 2021-12-07 17:34:26 PST
Jean-Yves Avenard [:jya]
Comment 5 2021-12-07 17:35:50 PST
Created attachment 446267 [details] Patch for EWS
Jean-Yves Avenard [:jya]
Comment 6 2021-12-09 06:36:38 PST
Created attachment 446537 [details] Patch rebase
Jean-Yves Avenard [:jya]
Comment 7 2021-12-09 06:37:42 PST
Created attachment 446538 [details] Patch for EWS
Jean-Yves Avenard [:jya]
Comment 8 2021-12-14 03:30:09 PST
Created attachment 447121 [details] Patch rebase
youenn fablet
Comment 9 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>&&?
Jean-Yves Avenard [:jya]
Comment 10 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
EWS
Comment 11 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].
Note You need to log in before you can comment on or make changes to this bug.