Bug 86540 – MHTML files should be loadable from all schemes considered local, not just file:

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Greg Spencer]

In MainResourceLoader::continueAfterContentPolicy and in MHTMLArchive::create, there are tests to make sure that we don't load remote html archives.  Both of these tests use KURL.isLocalFile(), which only checks to see if the scheme of the URL is "file:".

This is fine as a first approximation, but since the SchemeRegistry has the extensible mechanism shouldTreatURLSchemeAsLocal(), we should use that instead, since on some platforms we would like to be able to register non-file schemes as being local.

This is related to the Chromium bug: http://crbug.com/126955

[Greg Spencer]

Created attachment 142088 [details]
A proposed patch

[Greg Spencer]

Created attachment 142089 [details]
Patch

[WebKit Review Bot]

Attachment 142089 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:10:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Greg Spencer]

Created attachment 142093 [details]
Patch

[Greg Spencer]

Adding Adam as a potential reviewer.

[Adam Barth]

Comment on attachment 142093 [details]
Patch

Makes sense to me.

[WebKit Review Bot]

Comment on attachment 142093 [details]
Patch

Clearing flags on attachment: 142093

Committed r117206: <http://trac.webkit.org/changeset/117206>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by 86583

[Kenichi Ishibashi]

The patch also breaks chromium's browser_tests.
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#testType=browser_tests&tests=RenderViewHostTest.BaseURLParam

I'm going to revert the patch.

[Alexey Proskuryakov]

How does the change to MainResourceLoader::continueAfterContentPolicy affect WebArchives?

[Adam Barth]

Comment on attachment 142093 [details]
Patch

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

> Source/WebCore/loader/MainResourceLoader.cpp:270
> -            && !m_substituteData.isValid() && !url.isLocalFile();
> +            && !m_substituteData.isValid() && !SchemeRegistry::shouldTreatURLSchemeAsLocal(url);

Sorry, I gave you a bad review.  This function just take the scheme, not the whole URL:

http://trac.webkit.org/browser/trunk/Source/WebCore/platform/SchemeRegistry.cpp#L167

[Adam Barth]

> How does the change to MainResourceLoader::continueAfterContentPolicy affect WebArchives?

It should make it possible to load web archives from all "local" URLs, not just file URLs.  Normally, "file" is the only local URL scheme, but embedders can add other URL schemes that they want to work the same way as file URLs.

[Greg Spencer]

Comment on attachment 142093 [details]
Patch

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

>> Source/WebCore/loader/MainResourceLoader.cpp:270
>> +            && !m_substituteData.isValid() && !SchemeRegistry::shouldTreatURLSchemeAsLocal(url);
> 
> Sorry, I gave you a bad review.  This function just take the scheme, not the whole URL:
> 
> http://trac.webkit.org/browser/trunk/Source/WebCore/platform/SchemeRegistry.cpp#L167

Heh.  No, you didn't give me a bad review, I gave you bad code.  Clearly I should have run more of the test suite.  I'll upload a new patch today.

[Alexey Proskuryakov]

WebArchive behavior change makes me nervous, because these are always dangerous, and I don't understand our local schemes well enough. Perhaps Brady or Jessie could ponder whether it's safe.

[Adam Barth]

(In reply to comment #15)
> WebArchive behavior change makes me nervous, because these are always dangerous, and I don't understand our local schemes well enough. Perhaps Brady or Jessie could ponder whether it's safe.

One easy way to determine whether it's safe is to look at the list of schemes Safari registers as local.  I suspect it's just "file" and possibly a scheme for error pages (which won't have any WebArchive resources).

[Greg Spencer]

Created attachment 142540 [details]
Patch

[Greg Spencer]

OK, the latest patch should do the right thing, and the chromium browser tests that failed before are passing now (locally on my machine at least).

[Adam Barth]

(In reply to comment #18)
> OK, the latest patch should do the right thing, and the chromium browser tests that failed before are passing now (locally on my machine at least).

Have we resolved Alexey's concern?  To follow the approach in Comment #16, you can just build a debug version of the apple-mac port and set a breakpoint in the function that registers local schemes.  If you run that WebKit DLL in Safari, that will let us see what all Safari is registering

[Greg Spencer]

(In reply to comment #19)
> Have we resolved Alexey's concern?  To follow the approach in Comment #16,
> you can just build a debug version of the apple-mac port and set a
> breakpoint in the function that registers local schemes.  If you run that 
> WebKit DLL in Safari, that will let us see what all Safari is registering

Well, but that would mean I would have to have a mac with XCode and know
how to build the apple-mac version of WebKit. :-)

I can learn how to do that, but it'll take me a day or two to set up.

[Adam Barth]

If you remind me on Monday, I'd be happy to run that experiment for you.  (I'm currently traveling and don't have a build environment.)

[Jessie Berlin]

(In reply to comment #16)
> (In reply to comment #15)
> > WebArchive behavior change makes me nervous, because these are always dangerous, and I don't understand our local schemes well enough. Perhaps Brady or Jessie could ponder whether it's safe.
> 
> One easy way to determine whether it's safe is to look at the list of schemes Safari registers as local.  I suspect it's just "file" and possibly a scheme for error pages (which won't have any WebArchive resources).

I checked, and this is not a concern for Safari.

[Adam Barth]

Thanks Jessie.

[Adam Barth]

Comment on attachment 142540 [details]
Patch

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

> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:106
>      // For security reasons we only load MHTML pages from the local file system.

I'd change this comment slightly to say "from local URLs" rather than "from the local file system".

[Greg Spencer]

Created attachment 142756 [details]
Patch

[Greg Spencer]

(In reply to comment #24)
> I'd change this comment slightly to say "from local URLs" rather than "from the local file system".

OK, done.

[Greg Spencer]

Adam, is there anything else I need to do to commit this? (your "review+" doesn't seem to have done it, and I'm not familiar enough with WebKit submissions to know what's missing).

[Alexey Proskuryakov]

As described in <http://www.webkit.org/coding/contributing.html>, please set "commit-queue?" flag.

[Adam Barth]

Comment on attachment 142756 [details]
Patch

As ap says, you can set the commit-queue? flag to ask folks to mark your patch for commit.

[WebKit Review Bot]

Comment on attachment 142756 [details]
Patch

Clearing flags on attachment: 142756

Committed r118402: <http://trac.webkit.org/changeset/118402>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.