Bug 158991

Summary: Retrieving Blobs from IDB using cursors fails in WK2 (Sandboxing)
Product: WebKit Reporter: Brady Eidson <beidson>
Component: WebCore Misc.Assignee: Brady Eidson <beidson>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, buildbot, commit-queue, eric.carlson, jer.noble, joepeck, me, rniwa
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 149117    
Attachments:
Description Flags
Test #1
none
Test #2
none
Patch
none
Much better test that can be run in browser
none
Archive of layout-test-results from ews100 for mac-yosemite
none
Archive of layout-test-results from ews115 for mac-yosemite
none
Now with WK1 hopefully working none

Description Brady Eidson 2016-06-21 10:57:08 PDT
URL.createObjectURL from Blob saved in indexedDB does not work
Loading the URL gives a 404

From <rdar://problem/26882004>
Comment 1 Brady Eidson 2016-06-21 12:05:16 PDT
The reported steps:
Steps to Reproduce:
1. Save blob in indexedDB
2. Retrieve Blob
3. URL.createObjectURL(retreivedBlob)
4. Display Image (with an img tag)

Working on a repro case of this.
Comment 2 Brady Eidson 2016-06-21 12:10:18 PDT
We actually have layout tests that cover this (With XHR, admittedly, and not <img>) and they're working fine.

In the meantime I've pinged the originator and asked for a test case that reproduces.
Comment 3 David Heidrich 2016-06-21 15:23:39 PDT
Thank you for looking at the issue:

I created a Fiddle to show the Problem:

// Working: Store blob first and read directly:
https://jsfiddle.net/0y9vsfud/3/

// not Working: check if blob exists, load existing (results in 404), otherwise store
https://jsfiddle.net/0y9vsfud/11/

The first execution will just store the image, a second run will load the existing image from the database. This results in a broken image.

There are some other issues (not part of the Ticket), somehow I can't delete an existing database and the inspector does not show any items inside the collections.
Comment 4 Brady Eidson 2016-06-21 21:42:27 PDT
(In reply to comment #3)

> There are some other issues (not part of the Ticket)
> somehow I can't delete an existing database 

We have about 950 automated tests that successfully delete a database before they recreate it in the test, so what you're seeing here is definitely an edge case that we'd love more details on.

> and the inspector does not show any items inside the collections.

This is known. CC'ing Joe.
Comment 5 Brady Eidson 2016-06-22 09:57:41 PDT
Just a heads up, the fiddles don't work because of cross origin checks on the JS.

Putting them in single-file test cases works.
Comment 6 Brady Eidson 2016-06-22 09:59:47 PDT
(In reply to comment #5)
> Just a heads up, the fiddles don't work because of cross origin checks on
> the JS.
> 
> Putting them in single-file test cases works.

(You can see the exceptions in the web inspector console)
Comment 7 Brady Eidson 2016-06-22 10:00:29 PDT
In general, it's best for test cases to be standalone files you can attach to a bug.

I'm attaching your fiddles here.

For me, they both work in Safari Tech Preview and a nightly.
Comment 8 Brady Eidson 2016-06-22 10:00:54 PDT
Created attachment 281844 [details]
Test #1
Comment 9 Brady Eidson 2016-06-22 10:01:04 PDT
Created attachment 281845 [details]
Test #2
Comment 10 Brady Eidson 2016-06-22 10:01:58 PDT
Perhaps you were running into security origin issues when you saw things not working?

Could you give these test cases I attached a shot and see if they work for you?
Comment 11 David Heidrich 2016-06-22 10:07:28 PDT
Thank you. I disabled the security check I think. Sorry forgot to mention that.

Just saw that a new Preview Version has been released (Today). I tested this version with the second Attachement (OSX 10.11.4, Safari Version 9.1.2 (11601.7.4, 11602.1.37)). I still get the 404 on the second call. I will switch to another Computer and a fresh OSX install.
Comment 12 David Heidrich 2016-06-22 10:33:59 PDT
I tested Test #2 with another Machine, OS X 10.11.5 (15F34), Version 9.1.2 (11601.7.4, 11602.1.37), same Problem. Important: The first (initial) request always shows the webkit logo, the second Request should show the Safari Technology Preview Logo (but shows a failed image instead) and a 404 error is thrown for the blob url.
Comment 13 Brady Eidson 2016-06-22 11:44:31 PDT
I can reproduce that now, not sure why I couldn't before.
Comment 14 Brady Eidson 2016-06-23 09:39:05 PDT
In a debug build, Networking process is ASSERTing:

#0	0x00000001142d1db7 in ::WTFCrash() at /Volumes/Data/git/OpenSource/Source/WTF/wtf/Assertions.cpp:317
#1	0x000000010ff95f75 in WebKit::NetworkConnectionToWebProcess::getBlobDataFileReferenceForPath(WTF::String const&) at /Volumes/Data/git/OpenSource/Source/WebKit2/NetworkProcess/NetworkConnectionToWebProcess.cpp:285
#2	0x000000010fef11f7 in WebKit::NetworkBlobRegistry::registerBlobURLOptionallyFileBacked(WebKit::NetworkConnectionToWebProcess*, WebCore::URL const&, WebCore::URL const&, WTF::String const&) at /Volumes/Data/git/OpenSource/Source/WebKit2/NetworkProcess/FileAPI/NetworkBlobRegistry.cpp:89
#3	0x000000010ff95fed in WebKit::NetworkConnectionToWebProcess::registerBlobURLOptionallyFileBacked(WebCore::URL const&, WebCore::URL const&, WTF::String const&) at /Volumes/Data/git/OpenSource/Source/WebKit2/NetworkProcess/NetworkConnectionToWebProcess.cpp:291

...

RefPtr<WebCore::BlobDataFileReference> NetworkConnectionToWebProcess::getBlobDataFileReferenceForPath(const String& path)
{
    ASSERT(m_blobDataFileReferences.contains(path));
    return m_blobDataFileReferences.get(path);
}

m_blobDataFileReferences get's populated for a path from preregisterSandboxExtensionsForOptionallyFileBackedBlob, which is not getting hit by this point in time.

Basically, the Networking process either:
A - Isn't getting its sandbox extension for the file ever
B - Isn't getting it *yet* due to a race or out-of-order.
Comment 15 Brady Eidson 2016-06-23 09:40:49 PDT
WebProcess is getting WebIDBConnectionToServer::didGetRecord instead of WebIDBConnectionToServer::didGetRecordWithSandboxExtensions, so it never has any extensions to pass to Networking.
Comment 16 Brady Eidson 2016-06-23 09:54:38 PDT
(In reply to comment #15)
> WebProcess is getting WebIDBConnectionToServer::didGetRecord instead of
> WebIDBConnectionToServer::didGetRecordWithSandboxExtensions, so it never has
> any extensions to pass to Networking.

Aha!

Super obvious.

The test case uses a cursor, not an objectStore.get, and that's not getting the extra sandbox extension code.

Yikes!
Comment 17 Brady Eidson 2016-06-23 13:05:31 PDT
Have a patch that fixes this, just need to get the test case cleaned up as a layout test.
Comment 18 Brady Eidson 2016-06-23 14:01:24 PDT
Created attachment 281930 [details]
Patch
Comment 19 WebKit Commit Bot 2016-06-23 14:02:47 PDT
Attachment 281930 [details] did not pass style-queue:


ERROR: Source/WebKit2/DatabaseProcess/DatabaseProcess.h:79:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/DatabaseProcess/DatabaseProcess.h:127:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/DatabaseProcess/DatabaseProcess.cpp:287:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 3 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 20 David Heidrich 2016-06-23 15:05:10 PDT
Thank you a lot for fixing this :).
Comment 21 Brady Eidson 2016-06-23 16:50:18 PDT
Created attachment 281942 [details]
Much better test that can be run in browser
Comment 22 Brady Eidson 2016-06-23 16:50:47 PDT
(In reply to comment #20)
> Thank you a lot for fixing this :).

No, thanks for finding and reporting it so we have the opportunity to fix it!
Comment 23 WebKit Commit Bot 2016-06-23 16:52:09 PDT
Attachment 281942 [details] did not pass style-queue:


ERROR: Source/WebKit2/DatabaseProcess/DatabaseProcess.h:79:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/DatabaseProcess/DatabaseProcess.h:127:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/DatabaseProcess/DatabaseProcess.cpp:287:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 3 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 24 Brady Eidson 2016-06-23 17:25:44 PDT
Looks like WK1 test fails and I totally know why - I didn't allow opening multiple windows! sigh.

DRT needs it, even if WKTR doesn't
Comment 25 Build Bot 2016-06-23 17:39:32 PDT
Comment on attachment 281942 [details]
Much better test that can be run in browser

Attachment 281942 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1557769

New failing tests:
storage/indexeddb/modern/blob-cursor.html
Comment 26 Build Bot 2016-06-23 17:39:35 PDT
Created attachment 281947 [details]
Archive of layout-test-results from ews100 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 27 Build Bot 2016-06-23 17:59:11 PDT
Comment on attachment 281942 [details]
Much better test that can be run in browser

Attachment 281942 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1557833

New failing tests:
storage/indexeddb/modern/blob-cursor.html
Comment 28 Build Bot 2016-06-23 17:59:14 PDT
Created attachment 281948 [details]
Archive of layout-test-results from ews115 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 29 Brady Eidson 2016-06-23 18:50:35 PDT
Created attachment 281953 [details]
Now with WK1 hopefully working
Comment 30 WebKit Commit Bot 2016-06-23 18:51:49 PDT
Attachment 281953 [details] did not pass style-queue:


ERROR: Source/WebKit2/DatabaseProcess/DatabaseProcess.h:79:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/DatabaseProcess/DatabaseProcess.h:127:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/DatabaseProcess/DatabaseProcess.cpp:287:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 3 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 31 Brady Eidson 2016-06-23 20:35:13 PDT
Uggggh... remaining orange EWSes are in a "unable to build without patch, tree is red?" loop.
Comment 32 Brady Eidson 2016-06-23 20:42:11 PDT
Comment on attachment 281953 [details]
Now with WK1 hopefully working

Even though other platform EWSes are still orange, they're obviously not related to this patch.

CQ+'ing
Comment 33 WebKit Commit Bot 2016-06-23 21:09:02 PDT
Comment on attachment 281953 [details]
Now with WK1 hopefully working

Clearing flags on attachment: 281953

Committed r202414: <http://trac.webkit.org/changeset/202414>
Comment 34 WebKit Commit Bot 2016-06-23 21:09:07 PDT
All reviewed patches have been landed.  Closing bug.
Comment 35 David Heidrich 2016-07-13 15:02:26 PDT
I tested the latest Safari Preview (v8) and it seems to work, at least with Images (like png, jpeg, gif etc.). It does not work with Video and Sound Blobs (they won't play). I will try to create a test case.
Comment 36 Brady Eidson 2016-07-13 20:20:12 PDT
(In reply to comment #35)
> I tested the latest Safari Preview (v8) and it seems to work, at least with
> Images (like png, jpeg, gif etc.). It does not work with Video and Sound
> Blobs (they won't play). I will try to create a test case.

Can you demonstrate video and sounds blobs working *at all*?

That is, without Indexed DB usage, just directly applying a blob to a video/audio element?
Comment 37 Brady Eidson 2016-07-13 20:20:49 PDT
(In reply to comment #36)
> (In reply to comment #35)
> > I tested the latest Safari Preview (v8) and it seems to work, at least with
> > Images (like png, jpeg, gif etc.). It does not work with Video and Sound
> > Blobs (they won't play). I will try to create a test case.
> 
> Can you demonstrate video and sounds blobs working *at all*?
> 
> That is, without Indexed DB usage, just directly applying a blob to a
> video/audio element?

Regardless of the answer, when you put together a test case, please file a new bug.

The issue discovered in this bug has been resolved, and we prefer not to reuse bugs.