Bug 233923 - Make PreviewConverterProvider not modify the SharedBuffer once returned
Summary: Make PreviewConverterProvider not modify the SharedBuffer once returned
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jean-Yves Avenard [:jya]
URL:
Keywords: InRadar
Depends on:
Blocks: 233922
  Show dependency treegraph
 
Reported: 2021-12-07 04:07 PST by Jean-Yves Avenard [:jya]
Modified: 2021-12-14 15:06 PST (History)
40 users (show)

See Also:


Attachments
Patch for EWS (737.62 KB, patch)
2021-12-07 04:18 PST, Jean-Yves Avenard [:jya]
no flags Details | Formatted Diff | Diff
Patch (5.54 KB, patch)
2021-12-07 04:19 PST, Jean-Yves Avenard [:jya]
no flags Details | Formatted Diff | Diff
Patch (5.58 KB, patch)
2021-12-07 17:34 PST, Jean-Yves Avenard [:jya]
no flags Details | Formatted Diff | Diff
Patch for EWS (737.66 KB, patch)
2021-12-07 17:35 PST, Jean-Yves Avenard [:jya]
no flags Details | Formatted Diff | Diff
Patch (5.48 KB, patch)
2021-12-09 06:36 PST, Jean-Yves Avenard [:jya]
no flags Details | Formatted Diff | Diff
Patch for EWS (736.71 KB, patch)
2021-12-09 06:37 PST, Jean-Yves Avenard [:jya]
no flags Details | Formatted Diff | Diff
Patch (5.48 KB, patch)
2021-12-14 03:30 PST, Jean-Yves Avenard [:jya]
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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].