Bug 198805 - openDatabase should return an empty object when WebSQL is disabled
Summary: openDatabase should return an empty object when WebSQL is disabled
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sihui Liu
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-06-12 14:30 PDT by Sihui Liu
Modified: 2019-10-02 14:32 PDT (History)
14 users (show)

See Also:


Attachments
Patch (4.63 KB, patch)
2019-06-12 14:39 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (15.34 KB, patch)
2019-06-20 23:30 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (15.57 KB, patch)
2019-06-21 12:10 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (20.22 KB, patch)
2019-06-21 12:49 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-highsierra (3.21 MB, application/zip)
2019-06-21 14:13 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-highsierra-wk2 (2.76 MB, application/zip)
2019-06-21 14:18 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews115 for mac-highsierra (3.03 MB, application/zip)
2019-06-21 14:49 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews124 for ios-simulator-wk2 (2.65 MB, application/zip)
2019-06-21 14:59 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews210 for win-future (13.49 MB, application/zip)
2019-06-21 15:08 PDT, Build Bot
no flags Details
Patch for landing (26.41 KB, patch)
2019-06-21 22:07 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sihui Liu 2019-06-12 14:30:27 PDT
The quirk is made for sites that still use openDatabase as a check condition and not actually use Database to do stuff. 
In this case, we should not return a real Database object, because they are not supposed to do any WebSQL work when it's disabled.
Comment 1 Sihui Liu 2019-06-12 14:39:17 PDT
Created attachment 371987 [details]
Patch
Comment 2 Geoffrey Garen 2019-06-12 14:50:38 PDT
Comment on attachment 371987 [details]
Patch

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

I think we need a regression test for this. So, I think you'll also need to add something like window.internals.enableWebSQLQuirk() for testing.

> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:599
> +        ASSERT(name == "null");
> +        ASSERT(version == "null");
> +        ASSERT(displayName == "null");
> +        ASSERT(!estimatedSize);

I don't think we want to ASSERT here. Instead, we want to check whether all four arguments are null, and return an empty object if so, and otherwise continue with normal behavior. That way, "if (openDatabase(null, null, null, null)" will be true, and websites will stop assuming we're in private browsing mode.

> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:601
> +        return JSValue();

This line returns the C++ null JSValue(), which is only valid if you throw an exception, and will otherwise cause a crash.

To return an empty object, you can use the constructEmptyObject() helper function.
Comment 3 Sihui Liu 2019-06-20 23:30:52 PDT
Created attachment 372617 [details]
Patch
Comment 4 Build Bot 2019-06-21 01:28:49 PDT
Comment on attachment 372617 [details]
Patch

Attachment 372617 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/12539058

New failing tests:
mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-llint
apiTests
Comment 5 Sihui Liu 2019-06-21 12:10:47 PDT
Created attachment 372638 [details]
Patch
Comment 6 Sihui Liu 2019-06-21 12:49:12 PDT
Created attachment 372641 [details]
Patch
Comment 7 Geoffrey Garen 2019-06-21 13:14:17 PDT
Comment on attachment 372641 [details]
Patch

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

r=me with two changes

> Source/JavaScriptCore/runtime/JSFunction.h:83
> +    JS_EXPORT_PRIVATE static JSFunction* createUndefinedFunction(VM&, JSGlobalObject*, int length, const String& name, NativeFunction, Intrinsic = NoIntrinsic, NativeFunction nativeConstructor = callHostFunctionAsConstructor, const DOMJIT::Signature* = nullptr);

Let's call this createFunctionThatMasqueradesAsUndefined. This is a weird enough behavior that it deserves a detailed name.

> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:617
> +    if (!name)
> +        name = vm.propertyNames->anonymous.impl();

I don't think you need this null check. Is it needed for something? I'd suggest removing it.
Comment 8 Build Bot 2019-06-21 14:13:19 PDT
Comment on attachment 372641 [details]
Patch

Attachment 372641 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/12543574

New failing tests:
js/dom/global-function-resolve.html
Comment 9 Build Bot 2019-06-21 14:13:21 PDT
Created attachment 372648 [details]
Archive of layout-test-results from ews100 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 10 Build Bot 2019-06-21 14:18:05 PDT
Comment on attachment 372641 [details]
Patch

Attachment 372641 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/12543556

New failing tests:
js/dom/global-function-resolve.html
Comment 11 Build Bot 2019-06-21 14:18:07 PDT
Created attachment 372650 [details]
Archive of layout-test-results from ews106 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6
Comment 12 Build Bot 2019-06-21 14:49:53 PDT
Comment on attachment 372641 [details]
Patch

Attachment 372641 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12543653

New failing tests:
js/dom/global-function-resolve.html
Comment 13 Build Bot 2019-06-21 14:49:55 PDT
Created attachment 372653 [details]
Archive of layout-test-results from ews115 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 14 Build Bot 2019-06-21 14:59:45 PDT
Comment on attachment 372641 [details]
Patch

Attachment 372641 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/12543663

New failing tests:
js/dom/global-function-resolve.html
Comment 15 Build Bot 2019-06-21 14:59:47 PDT
Created attachment 372654 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.14.5
Comment 16 Build Bot 2019-06-21 15:08:45 PDT
Comment on attachment 372641 [details]
Patch

Attachment 372641 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12543812

New failing tests:
js/dom/global-function-resolve.html
Comment 17 Build Bot 2019-06-21 15:08:48 PDT
Created attachment 372655 [details]
Archive of layout-test-results from ews210 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews210  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment 18 Sihui Liu 2019-06-21 22:07:27 PDT
Created attachment 372673 [details]
Patch for landing
Comment 19 WebKit Commit Bot 2019-06-21 22:50:09 PDT
Comment on attachment 372673 [details]
Patch for landing

Clearing flags on attachment: 372673

Committed r246707: <https://trac.webkit.org/changeset/246707>
Comment 20 WebKit Commit Bot 2019-06-21 22:50:11 PDT
All reviewed patches have been landed.  Closing bug.
Comment 21 Radar WebKit Bug Importer 2019-06-21 22:52:44 PDT
<rdar://problem/52013842>
Comment 22 Ryosuke Niwa 2019-06-24 20:02:08 PDT
Ah, this is a very neat trick!
Comment 23 Oz 2019-10-02 02:04:16 PDT
This change broke:

  if (typeof window.openDatabase == "function") {
    // use websql
  } else {
    // dont use websql
  }

Also created a paradox

  typeof window.openDatabase == "undefined"
  typeof window.openDatabase == "function"

both true

  typeof window.openDatabase == "something-else"

false as expected.

https://stackoverflow.com/questions/58107791/detecting-websql-support-or-lack-thereof-in-safari-13

The workaround was simple.  
Use !!window.openDatabase to test for presence of the API.
Comment 24 Geoffrey Garen 2019-10-02 11:20:01 PDT
Filed the typeof bug as https://bugs.webkit.org/show_bug.cgi?id=202485.
Comment 25 Ryosuke Niwa 2019-10-02 12:46:35 PDT
Oz, do you know of any popular website that got broken due to the typeof issue?
Comment 26 Oz 2019-10-02 13:21:32 PDT
(In reply to Ryosuke Niwa from comment #25)
> Oz, do you know of any popular website that got broken due to the typeof
> issue?

Our mobile app when run in iOS safari with websql advanced feature disabled (the default in iOS 13) or as iOS standalone web app (home screen icon).  I have worked around the issue by now testing !!window.openDatabase.

I don't know how common it is to test for the API using

  typeof window.openDatabase == "function" 

But it seemed a legit way to test for presence of the API and works in all other browsers that I have tested and worked prior to safari 13.

https://github.com/search?q=%22typeof+window.openDatabase+%3D%3D+function%22&type=Code

throws up a fair number of uses of typeof window.openDatabase == "function" selenium webdriver at some point being one example (a lot of forks use this test)

https://github.com/SeleniumHQ/selenium/search?q=typeof+window.openDatabase&unscoped_q=typeof+window.openDatabase

It seems much less common than typeof window.openDatabase !== "undefined" however

https://github.com/search?q=%22typeof+window.openDatabase+%3D%3D%22&type=Code
(14,000 hits)

Which I presume will also fail to work as expected given typeof window.openDatabase == "undefined" after this change.
Comment 27 Oz 2019-10-02 13:24:22 PDT
Copied from the SO link above, a test page I knocked up to replicate the issue.

http://locutus.sorcerer.co.uk/demo/safari-openDatabase.html
Comment 28 Ryosuke Niwa 2019-10-02 13:52:27 PDT
(In reply to Oz from comment #26)
> (In reply to Ryosuke Niwa from comment #25)
> > Oz, do you know of any popular website that got broken due to the typeof
> > issue?
> 
> Our mobile app when run in iOS safari with websql advanced feature disabled
> (the default in iOS 13) or as iOS standalone web app (home screen icon).  I
> have worked around the issue by now testing !!window.openDatabase.

What is your app (URL)? Is it an iOS app that embeds WKWebView or opens Safari? Or is it a web app that gets loaded in Safari or saved to home screen?
Comment 29 Oz 2019-10-02 14:32:33 PDT
(In reply to Ryosuke Niwa from comment #28)
> (In reply to Oz from comment #26)
> > (In reply to Ryosuke Niwa from comment #25)
> > > Oz, do you know of any popular website that got broken due to the typeof
> > > issue?
> > 
> > Our mobile app when run in iOS safari with websql advanced feature disabled
> > (the default in iOS 13) or as iOS standalone web app (home screen icon).  I
> > have worked around the issue by now testing !!window.openDatabase.
> 
> What is your app (URL)? Is it an iOS app that embeds WKWebView or opens
> Safari? Or is it a web app that gets loaded in Safari or saved to home
> screen?

The app runs in a various environments. The environments in which testing typeof window.openDatabase is an issue are:

iOS 13 - Safari (which disable webSQL experimental feature enabled)
iOS 13 - iOS standalone web app (ie home screen icon)

There is no public URL, in the above two cases the app is served off an internal web server, most likely over http.

The public offering (as in app store/play store published version) is a Cordova based app that uses WebView and is unaffected by this change as WebView in iOS 13 still supports WebSQL.