Bug 202012 – Regression(r248832): Unable to quicklook HTML files in Mail

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Chris Dumez]

Unable to quicklook HTML files in Mail due to sandboxing since r248832.

[Chris Dumez]

<rdar://problem/55285295>

[Chris Dumez]

Created attachment 379171 [details]
Patch

[Chris Dumez]

Created attachment 379172 [details]
Patch

[Chris Dumez]

Created attachment 379173 [details]
Patch

[Brent Fulgham]

Comment on attachment 379173 [details]
Patch

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

Thank you for jumping on this bug. r=me

> Source/WebKit/UIProcess/AuxiliaryProcessProxy.cpp:-225
> -            }

Is this the Mail case?

> Source/WebKit/UIProcess/WebPageProxy.cpp:1186
> +    if (!process->isLaunching() || !url.isLocalFile())

Why don't we call into the WebPageProxy::loadFile code path when the url is a local file? Are there cases that are "isLocalFile" but shouldn't use the file loading path?

> Source/WebKit/UIProcess/WebPageProxy.cpp:1241
> +    if (!m_process->isLaunching())

This would be clearer if we flipped the check:

if (m_process->isLaunching())
    ... Send the 'LoadRequestWaitingForPID' message
else
    .... Send the 'LoadRequest' message.

But I guess that is the reverse of the "load from network' case.

[Geoffrey Garen]

Comment on attachment 379173 [details]
Patch

r=me

Can we regression test this? (Maybe possible through an API test that issues a file URL load?)

> Source/WebKit/ChangeLog:10
> +        the same logic to initializing the sandbox extension if the process has already

to initialize

had already

> Source/WebKit/ChangeLog:13
> +        the client did not provided one. The logic in maybeInitializeSandboxExtensionHandle()

provide

[Chris Dumez]

(In reply to Geoffrey Garen from comment #6)
> Comment on attachment 379173 [details]
> Patch
> 
> r=me
> 
> Can we regression test this? (Maybe possible through an API test that issues
> a file URL load?)

No because our API tests are not sandboxed.

> 
> > Source/WebKit/ChangeLog:10
> > +        the same logic to initializing the sandbox extension if the process has already
> 
> to initialize
> 
> had already
> 
> > Source/WebKit/ChangeLog:13
> > +        the client did not provided one. The logic in maybeInitializeSandboxExtensionHandle()
> 
> provide

[Chris Dumez]

Comment on attachment 379173 [details]
Patch

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

>> Source/WebKit/UIProcess/AuxiliaryProcessProxy.cpp:-225
>> -            }
> 
> Is this the Mail case?

The mail case is https://trac.webkit.org/changeset/247400/webkit, which is in maybeInitializeSandboxExtensionHandle(). This is why I am now calling maybeInitializeSandboxExtensionHandle().

>> Source/WebKit/UIProcess/WebPageProxy.cpp:1186
>> +    if (!process->isLaunching() || !url.isLocalFile())
> 
> Why don't we call into the WebPageProxy::loadFile code path when the url is a local file? Are there cases that are "isLocalFile" but shouldn't use the file loading path?

I would need to look into it more to see if there would be side effects from calling loadFile(). In any case, I'd rather not do this in this patch to reduce risk.

>> Source/WebKit/UIProcess/WebPageProxy.cpp:1241
>> +    if (!m_process->isLaunching())
> 
> This would be clearer if we flipped the check:
> 
> if (m_process->isLaunching())
>     ... Send the 'LoadRequestWaitingForPID' message
> else
>     .... Send the 'LoadRequest' message.
> 
> But I guess that is the reverse of the "load from network' case.

Ok.

[Chris Dumez]

Created attachment 379174 [details]
Patch

[Chris Dumez]

Comment on attachment 379174 [details]
Patch

Clearing flags on attachment: 379174

Committed r250110: <https://trac.webkit.org/changeset/250110>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Per Arne Vollan]

Comment on attachment 379174 [details]
Patch

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

> Source/WebKit/UIProcess/WebPageProxy.cpp:1239
>  
>      m_process->assumeReadAccessToBaseURL(*this, resourceDirectoryURL);

I think this line should be removed, since this is handled inside maybeInitializeSandboxExtensionHandle now, and can cause a later call of maybeInitializeSandboxExtensionHandle to just bail if read access is already assumed.

[Chris Dumez]

(In reply to Per Arne Vollan from comment #12)
> Comment on attachment 379174 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=379174&action=review
> 
> > Source/WebKit/UIProcess/WebPageProxy.cpp:1239
> >  
> >      m_process->assumeReadAccessToBaseURL(*this, resourceDirectoryURL);
> 
> I think this line should be removed, since this is handled inside
> maybeInitializeSandboxExtensionHandle now, and can cause a later call of
> maybeInitializeSandboxExtensionHandle to just bail if read access is already
> assumed.

Good catch, thanks.

[Chris Dumez]

Reopen to fix issue found by Per Arne.

[Chris Dumez]

Created attachment 379238 [details]
Patch

[Per Arne Vollan]

Comment on attachment 379238 [details]
Patch

Thanks for fixing this bug! R=me.

[WebKit Commit Bot]

Comment on attachment 379238 [details]
Patch

Clearing flags on attachment: 379238

Committed r250130: <https://trac.webkit.org/changeset/250130>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.