Bug 158991 – Retrieving Blobs from IDB using cursors fails in WK2 (Sandboxing)

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

[Brady Eidson]

URL.createObjectURL from Blob saved in indexedDB does not work
Loading the URL gives a 404

From <rdar://problem/26882004>

[Brady Eidson]

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.

[Brady Eidson]

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.

[David Heidrich]

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.

[Brady Eidson]

(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.

[Brady Eidson]

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.

[Brady Eidson]

(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)

[Brady Eidson]

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.

[Brady Eidson]

Created attachment 281844 [details]
Test #1

[Brady Eidson]

Created attachment 281845 [details]
Test #2

[Brady Eidson]

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?

[David Heidrich]

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.

[David Heidrich]

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.

[Brady Eidson]

I can reproduce that now, not sure why I couldn't before.

[Brady Eidson]

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.

[Brady Eidson]

WebProcess is getting WebIDBConnectionToServer::didGetRecord instead of WebIDBConnectionToServer::didGetRecordWithSandboxExtensions, so it never has any extensions to pass to Networking.

[Brady Eidson]

(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!

[Brady Eidson]

Have a patch that fixes this, just need to get the test case cleaned up as a layout test.

[Brady Eidson]

Created attachment 281930 [details]
Patch

[WebKit Commit Bot]

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.

[David Heidrich]

Thank you a lot for fixing this :).

[Brady Eidson]

Created attachment 281942 [details]
Much better test that can be run in browser

[Brady Eidson]

(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!

[WebKit Commit Bot]

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.

[Brady Eidson]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Brady Eidson]

Created attachment 281953 [details]
Now with WK1 hopefully working

[WebKit Commit Bot]

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.

[Brady Eidson]

Uggggh... remaining orange EWSes are in a "unable to build without patch, tree is red?" loop.

[Brady Eidson]

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

[WebKit Commit Bot]

Comment on attachment 281953 [details]
Now with WK1 hopefully working

Clearing flags on attachment: 281953

Committed r202414: <http://trac.webkit.org/changeset/202414>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[David Heidrich]

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.

[Brady Eidson]

(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?

[Brady Eidson]

(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.