[webkit-reviews] review requested: [Bug 210208] Fix handling non-linearized PDFs when incremental PDF loading is enabled : [Attachment 395852] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has asked  for review:
Bug 210208: Fix handling non-linearized PDFs when incremental PDF loading is
enabled
https://bugs.webkit.org/show_bug.cgi?id=210208

Attachment 395852: Patch

https://bugs.webkit.org/attachment.cgi?id=395852&action=review




--- Comment #4 from Geoffrey Garen <ggaren at apple.com> ---
Comment on attachment 395852
  --> https://bugs.webkit.org/attachment.cgi?id=395852
Patch

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

> Source/WebKit/ChangeLog:9
> +	   When we try to load a non-linearized PDF with PDFKit, it makes an
outlandishly large range request
> +	   to try to verify the PDF file size.

Are we working around a bug in PDFKit here? If so, let's cite the Radar for
that bug.

> Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:142
> +static const int32_t maximumRangeRequestPosition =
std::numeric_limits<int32_t>::max();

Should this be off_t, since the position argument passed to
dataProviderGetBytesAtPositionCallback is off_t?

What is the actual request we get from PDFKit in practice? Maybe we can bump
this number up, and do a direct comparison instead of a > comparison. This
limit would prohibit streaming of any > 4GB PDF resource. While 4GB is very
big, it doesn't strike me as impossibly big, and refusing to stream something
because it was big would be a sad paradox.

> Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:701
> +void PDFPlugin::receivedInvalidRangeRequest()

I think this code would be clearer if we named and/or commented it according to
the behavior we are working around / modeling. Maybe something like

    maximumRangeRequestPosition => nonLinearizedPDFSentinel
    receivedInvalidRangeRequest => receivedNonLinearizedPDFSentinel

> Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:741
> +    Ref<PDFPlugin> plugin = *((PDFPlugin*)info);

Let's put this at the top of the function so we can remove the casting above.

> Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:744
> +    // It's possible we previously encountered an invalid range and
therefore disabled incremental loading,
> +    // but PDFDocument is still trying to read data using a different
strategy.

Does this happen?

> Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:750
> +    auto debugPluginRef = plugin.copyRef();

I think you can just use plugin here.

> Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:859
> +	   // The main thread dispatch below removes the last reference to the
PDF thread.
> +	   // It must be the last code executed in this function.
> +	   callOnMainThread([protectedPlugin = WTFMove(protectedPlugin)] { });

callOnMainThread can execute its function concurrently at any time; so, this is
a suspicious lifetime idiom, and probably not fully correct. What are we trying
to accomplish here?