Bug 237120 – WebKit continues to render PDF images in Captive Portal mode

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Brent Fulgham]

We should hand PDF off to PDF.js when CaptivePortal mode is enabled. However, images continue to be processed using native PDF decoding.

[Radar WebKit Bug Importer]

<rdar://problem/89384234>

[Brent Fulgham]

This should not work in CaptivePortal mode, but currently renders:

<img width="150px" height="100px" src="images/webkit-logo.pdf">

[Brent Fulgham]

Created attachment 453050 [details]
Patch

[Brent Fulgham]

Created attachment 453157 [details]
Patch

[Chris Dumez]

Comment on attachment 453157 [details]
Patch

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

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:122
> +        return new CachedImage(WTFMove(request), sessionID, cookieJar, settings.captivePortalModeEnabled() ? Image::UseRestrictedDecoder::Yes : Image::UseRestrictedDecoder::No);

This is a bit weird since settings are per page but CachedImages can totally be shared between pages. The MemoryCache (which stores the CachedImages) is a singleton object inside the process.
As a matter of fact, cachePortalMode is process-global too. As a result, it seems we're passing flags around and adding a data member to CachedImage unnecessarily.

It makes it look like we could support one page have restricted decoder for one page but not for another in the same process, which is just not possible at the moment since they may share the same CachedImage if both pages were to display the same image.

As a result, my suggestion would be to not add a CaptivePortalModeEnabled setting on the page. Instead, it should be some kind of global flag.
Maybe a static variable on Image or a flag on RuntimeEnabledFeatures (which is a singleton), or somewhere else that is global.

Also note that until now, our flags were about specific features, WebCore was not aware of captive portal mode, which I thought was nice to keep at the WebKit/Process model layer.
So maybe the flag could be about PDF images?

[Brent Fulgham]

Created attachment 453248 [details]
Patch

[Chris Dumez]

Comment on attachment 453248 [details]
Patch

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

r=me with changes.

> Source/WebCore/page/RuntimeEnabledFeatures.h:204
> +    void setPDFImagesEnabled(bool isEnabled) { m_pdfImagesEnabled = isEnabled; }

Per WebKit coding style, Boolean variables need a prefix. Should be something like:
void setArePDFImagesEnabled(bool arePDFImagesEnabled) { m_arePDFImagesEnabled = arePDFImagesEnabled; }

> Source/WebCore/page/RuntimeEnabledFeatures.h:205
> +    bool pdfImagesEnabled() const { return m_pdfImagesEnabled; }

bool arePDFImagesEnabled() const { return m_arePDFImagesEnabled; }

> Source/WebCore/page/RuntimeEnabledFeatures.h:360
> +    bool m_pdfImagesEnabled { true };

ditto.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:4074
> +        RuntimeEnabledFeatures::sharedFeatures().setPDFImagesEnabled(false);

This has nothing to do with the page, it is process-wide.
As a result, I think this call should be in the WebProcess constructor, right after we initialize WebProcess::m_isCaptivePortalModeEnabled.

> Tools/ChangeLog:20
> +        * TestWebKitAPI/Tests/WebKitCocoa/missing.png: Added.

I think we can probably omit this one.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/CaptivePortalPDF.html:3
> +        <style>

Why the styling for an API test?

> Tools/TestWebKitAPI/Tests/WebKitCocoa/CaptivePortalPDF.html:20
> +        <h1>Captive Portal Mode - PDF Test</h1>

This is not a layout test and I don't think we need so much HTML code for an API test. I don't think these tests are expected to work when loading manually in the browser.
It's not like the window.webkit.messageHandlers.testHandler would exist in the browser anyway.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/CaptivePortalPDF.html:30
> +                <td><img width="150px" height="100px" src="missing.png"></td>

What's the purpose on the second image in this test? Seems like we could just omit it, or am I missing something?

[Brent Fulgham]

Comment on attachment 453248 [details]
Patch

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

>> Source/WebCore/page/RuntimeEnabledFeatures.h:204
>> +    void setPDFImagesEnabled(bool isEnabled) { m_pdfImagesEnabled = isEnabled; }
> 
> Per WebKit coding style, Boolean variables need a prefix. Should be something like:
> void setArePDFImagesEnabled(bool arePDFImagesEnabled) { m_arePDFImagesEnabled = arePDFImagesEnabled; }

Will do!

>> Source/WebKit/WebProcess/WebPage/WebPage.cpp:4074
>> +        RuntimeEnabledFeatures::sharedFeatures().setPDFImagesEnabled(false);
> 
> This has nothing to do with the page, it is process-wide.
> As a result, I think this call should be in the WebProcess constructor, right after we initialize WebProcess::m_isCaptivePortalModeEnabled.

Ah! Good point -- will fix.

>> Tools/ChangeLog:20
>> +        * TestWebKitAPI/Tests/WebKitCocoa/missing.png: Added.
> 
> I think we can probably omit this one.

Will do.

[Brent Fulgham]

Created attachment 453265 [details]
Patch for landing

[EWS]

Committed r290534 (247813@main): <https://commits.webkit.org/247813@main>

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