Bug 56328 – LayoutTests exercising filesystem: URIs should run over HTTP to avoid weirdness related to file: URLs

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Adam Klein]

Reported by ericu.

Test output in Chromium:
--------------------------------
Tests using resolveLocalFileSystemURI to obtain an Entry from a URI

On success, you will see a series of "PASS" messages, followed by
"TEST COMPLETE".


* Resolving a generated URI.
PASS expectedPath is actualPath
PASS isFile is true
* Resolving test file by hand
PASS expectedPath is actualPath
PASS isFile is true
* Resolving a completely bogus URI.
PASS
* Resolving a URI with the wrong protocol
PASS
* Resolving a URI with no slash between type and file
PASS
* Resolving a URI with no slash between protocol and type
PASS
* Resolve a path using backslashes
PASS expectedPath is actualPath
PASS isFile is true
* Resolve a directory
PASS expectedPath is actualPath
PASS isDirectory is true
* Resolve a path using a trailing slash
Error occurred: 2
FAIL Bailing out early
PASS successfullyParsed is true

TEST COMPLETE
---------------------------------------

Here's the failing call stack.  canRequest is returning false because
targetOrigin->isUnique() returns true.  It's trying to prevent file
URLs from mixing with other urls, I think.  I'm guessing we need a
special case here, as with the Blob protocol, but I don't know what
the minimal hole to open is.

       chrome.dll!WebCore::SecurityOrigin::canRequest(const WebCore::KURL & url={...})  Line 262       C++
       chrome.dll!WebCore::DOMWindow::resolveLocalFileSystemURI(const
WTF::String & uri={...}, WTF::PassRefPtr<WebCore::EntryCallback>
successCallback={...}, WTF::PassRefPtr<WebCore::ErrorCallback>
errorCallback={...})  Line 766 + 0x27 bytes     C++
       chrome.dll!WebCore::DOMWindowInternal::resolveLocalFileSystemURICallback(const
v8::Arguments & args={...})  Line 3046 + 0x30 bytes     C++
       chrome.dll!v8::internal::HandleApiCallHelper<0>(v8::internal::`anonymous-namespace'::BuiltinArguments<1>
args={...})  Line 1069 + 0x13 bytes     C++
       chrome.dll!v8::internal::Builtin_Impl_HandleApiCall(v8::internal::`anonymous-namespace'::BuiltinArguments<1>
args={...})  Line 1086 + 0xd bytes      C++
       chrome.dll!v8::internal::Builtin_HandleApiCall(v8::internal::`anonymous-namespace'::BuiltinArguments<1>
args={...})  Line 1085 + 0x18 bytes     C++
       0ff702ae()
       chrome.dll!v8::internal::Invoke(bool construct=false,
v8::internal::Handle<v8::internal::JSFunction> func={...},
v8::internal::Handle<v8::internal::Object> receiver={...}, int argc=1,
v8::internal::Object * * * args=0x001ae734, bool *
has_pending_exception=0x001ae61b)  Line 97 + 0x19 bytes C++
       chrome.dll!v8::internal::Execution::Call(v8::internal::Handle<v8::internal::JSFunction>
func={...}, v8::internal::Handle<v8::internal::Object> receiver={...},
int argc=1, v8::internal::Object * * * args=0x001ae734, bool *
pending_exception=0x001ae61b)  Line 128 + 0x1f bytes    C++
       chrome.dll!v8::Function::Call(v8::Handle<v8::Object> recv={...}, int
argc=1, v8::Handle<v8::Value> * argv=0x001ae734)  Line 2928 + 0x1d
bytes   C++
       chrome.dll!WebCore::invokeCallback(v8::Persistent<v8::Object>
callback={...}, int argc=1, v8::Handle<v8::Value> * argv=0x001ae734,
bool & callbackReturnValue=false, WebCore::ScriptExecutionContext *
scriptExecutionContext=0x015b7038)  Line 86 + 0x1f bytes        C++
       chrome.dll!WebCore::V8EntryCallback::handleEvent(WebCore::Entry *
entry=0x026a1460)  Line 76 + 0x22 bytes C++
       chrome.dll!WebCore::EntryCallbacks::didSucceed()  Line 132 + 0x47 bytes C++
       chrome.dll!WebKit::WebFileSystemCallbacksImpl::didSucceed()  Line 65
+ 0x1c bytes    C++
       chrome.dll!WebFileSystemCallbackDispatcher::DidSucceed()  Line 32 +
0x16 bytes      C++
       chrome.dll!FileSystemDispatcher::OnDidSucceed(int request_id=182)
Line 233 + 0xf bytes    C++
       chrome.dll!DispatchToMethod<FileSystemDispatcher,void (__thiscall
FileSystemDispatcher::*)(int),int>(FileSystemDispatcher *
obj=0x015135a0, void (int)* method=0x5d2cf0a0, const Tuple1<int> &
arg={...})  Line 551 + 0xe bytes        C++
       chrome.dll!IPC::MessageWithTuple<Tuple1<int>
>::Dispatch<FileSystemDispatcher,FileSystemDispatcher,void (__thiscall
FileSystemDispatcher::*)(int)>(const IPC::Message * msg=0x014f5da8,
FileSystemDispatcher * obj=0x015135a0, FileSystemDispatcher *
sender=0x015135a0, void (int)* func=0x5d2cf0a0)  Line 945 + 0x11
bytes   C++
       chrome.dll!FileSystemDispatcher::OnMessageReceived(const
IPC::Message & msg={...})  Line 30 + 0x16 bytes C++
       chrome.dll!ChildThread::OnMessageReceived(const IPC::Message &
msg={...})  Line 147 + 0x2d bytes       C++
       chrome.dll!IPC::ChannelProxy::Context::OnDispatchMessage(const
IPC::Message & message={...})  Line 255 + 0x19 bytes    C++
       chrome.dll!DispatchToMethod<IPC::ChannelProxy::Context,void
(__thiscall IPC::ChannelProxy::Context::*)(IPC::Message const
&),IPC::Message>(IPC::ChannelProxy::Context * obj=0x015341e0, void
(const IPC::Message &)* method=0x5d294820, const Tuple1<IPC::Message>
& arg={...})  Line 551 + 0xf bytes      C++
       chrome.dll!RunnableMethod<IPC::ChannelProxy::Context,void
(__thiscall IPC::ChannelProxy::Context::*)(IPC::Message const
&),Tuple1<IPC::Message> >::Run()  Line 331 + 0x1e bytes C++
       chrome.dll!MessageLoop::RunTask(Task * task=0x014f5d80)  Line 367 +
0xf bytes       C++
       chrome.dll!MessageLoop::DeferOrRunPendingTask(const
MessageLoop::PendingTask & pending_task={...})  Line 379        C++
       chrome.dll!MessageLoop::DoWork()  Line 569 + 0xc bytes  C++
       chrome.dll!base::MessagePumpDefault::Run(base::MessagePump::Delegate
* delegate=0x001af0bc)  Line 23 + 0xf bytes     C++
       chrome.dll!MessageLoop::RunInternal()  Line 342 + 0x2a bytes    C++
       chrome.dll!MessageLoop::RunHandler()  Line 316  C++
       chrome.dll!MessageLoop::Run()  Line 240 C++
       chrome.dll!RendererMain(const MainFunctionParams & parameters={...})
 Line 315       C++
       chrome.dll!`anonymous namespace'::RunNamedProcessTypeMain(const
std::basic_string<char,std::char_traits<char>,std::allocator<char> > &
process_type="renderer", const MainFunctionParams &
main_function_params={...})  Line 453 + 0x12 bytes      C++
       chrome.dll!ChromeMain(HINSTANCE__ * instance=0x009b0000,
sandbox::SandboxInterfaceInfo * sandbox_info=0x001afa10, wchar_t *
command_line_unused=0x0051345a)  Line 766 + 0x13 bytes  C++
       chrome.exe!MainDllLoader::Launch(HINSTANCE__ * instance=0x009b0000,
sandbox::SandboxInterfaceInfo * sbox_info=0x001afa10)  Line 280 + 0x1d
bytes   C++
       chrome.exe!wWinMain(HINSTANCE__ * instance=0x009b0000, HINSTANCE__ *
__formal=0x00000000, HINSTANCE__ * __formal=0x00000000, HINSTANCE__ *
__formal=0x00000000)  Line 46 + 0x10 bytes      C++
       chrome.exe!__tmainCRTStartup()  Line 263 + 0x2c bytes   C
       chrome.exe!wWinMainCRTStartup()  Line 182       C
       kernel32.dll!75d9e4a5()
       [Frames below may be incorrect and/or missing, no symbols loaded for
kernel32.dll]
       ntdll.dll!777acfed()
       ntdll.dll!777ad1ff()

[Adam Klein]

When running under DumpRenderTree, SecurityOrigin::canRequest gets short-circuited at

    if (m_universalAccess)
        return true;

LayoutTestController seems to provide access to this boolean, but my first attempt to simply make it false at the start of the test didn't have the expected effect.  Digging more.

[Adam Klein]

Tracing this up the chain, m_universalAccess is true because of this snippet from Tools/DumpRenderTree/chromium/WebPreferences.cpp:

    // Allow those layout tests running as local files, i.e. under
    // LayoutTests/http/tests/local, to access http server.
    allowUniversalAccessFromFileURLs = true;

[Adam Klein]

What's needed to get this test operating appropriately in DumpRenderTree is to somehow run the test inside a new document: the layoutTestController method has no effect on the current document.

[Adam Klein]

And finally, the culprit, in SecurityOrigin's constructor:

101     // By default, only local SecurityOrigins can load local resources.
102     m_canLoadLocalResources = isLocal();
103     if (m_canLoadLocalResources) {
104         // Directories should never be readable.
105         if (!url.hasPath() || url.path().endsWith("/"))
106             m_isUnique = true;
107         // Store the path in case we are doing per-file origin checking.
108         m_filePath = url.path();
109     }

Note the url.path().endsWith("/") check.  Not sure the best way to work around this, will need to think about this.  CCing Adam Barth (though I suspect I'll need to chat with him to provide sufficient context).

[Adam Barth]

Yeah, sorry about enabling universal access for file URLs during testing.  We have that set that way because tons of tests expect it.

Is there an actual bug here, or just that the tests don't run correctly outside of DRT?  Many, many non-HTTP tests don't run correctly outside of DRT for this reason.

[Adam Klein]

(In reply to comment #5)
> Yeah, sorry about enabling universal access for file URLs during testing.  We have that set that way because tons of tests expect it.
> 
> Is there an actual bug here, or just that the tests don't run correctly outside of DRT?  Many, many non-HTTP tests don't run correctly outside of DRT for this reason.

There is an actual bug here (the title of this bug is vague because I didn't know what the problem was, but wanted somewhere to post my findings).

The issue is that when we call SecurityOrigin::canRequest() in requestLocalFileSystemURI, URIs with trailing slashes are rejected.  This is due to SecurityOrigin::isLocal() being true for filesystem: URIs, combined with the rule that local directories should never be requestable (see the code snippet in my previous post). 

Now, it may or may not be a bug that directories aren't fetchable with, say, an iframe (there's been some discussion of this within the team).  But we certainly want to be able to _resolve_ directories, since that'll just return a DirectoryEntry which JS definitely can make use of.  I think the easiest fix here is to not check SecurityOrigin::canRequest, but instead do a simpler check for "resolvability", which I think should just be SecurityOrigin::isSameSchemeHostPort(SecurityOrigin::create(fileSystemURL)).  Does the latter seem reasonable?

That still won't let directory listings be requested via iframe or XMLHTTPRequest, so perhaps that needs a separate bug.

[Adam Barth]

> The issue is that when we call SecurityOrigin::canRequest() in requestLocalFileSystemURI, URIs with trailing slashes are rejected.  This is due to SecurityOrigin::isLocal() being true for filesystem: URIs, 

^^^  That's the bug.  SecurityOrigin::isLocal() should not be true for filesystem: URIs.

[Adam Klein]

(In reply to comment #7)
> > The issue is that when we call SecurityOrigin::canRequest() in requestLocalFileSystemURI, URIs with trailing slashes are rejected.  This is due to SecurityOrigin::isLocal() being true for filesystem: URIs, 
> 
> ^^^  That's the bug.  SecurityOrigin::isLocal() should not be true for filesystem: URIs.

Okay, so sounds like the right thing to do is move all these tests under LayoutTests/http to make them behave like they do in the "real world" (that is, in Chrome without the --allow-file-access-from-files flag).  Retitling appropriately.

[Adam Klein]

Created attachment 85843 [details]
Patch

[Adam Klein]

Note that this patch depends on http://codereview.chromium.org/6695030/, and should not be committed before that both lands and rolls into WebKit's Chromium DEPS.

[Adam Barth]

Comment on attachment 85843 [details]
Patch

rs=me

[Adam Klein]

Created attachment 85956 [details]
Includes DEPs roll

[Adam Klein]

svn-apply doesn't seem to like my patches.  Is this going to pose a problem for the commit-queue?  Am I doing something wrong wrt moving/modifying files?

[Eric U.]

Hmm...I also had problems with a move+modify CL.  Commit queue failed, and wouldn't ever tell me why.  I eventually had to land it by hand.

[Adam Barth]

Committed r81297: <http://trac.webkit.org/changeset/81297>

[Adam Barth]

(In reply to comment #14)
> Hmm...I also had problems with a move+modify CL.  Commit queue failed, and wouldn't ever tell me why.  I eventually had to land it by hand.

Yeah, for some reason svn-apply doesn't handle these cases correctly.  If you're interested in improving the tools, the code that needs fixing is likely in http://trac.webkit.org/browser/trunk/Tools/Scripts/VCSUtils.pm.

In any case, I've landed this patch manually.

[WebKit Review Bot]

http://trac.webkit.org/changeset/81297 might have broken SnowLeopard Intel Release (Tests)
The following tests are not passing:
http/tests/filesystem/resolve-uri.html