openDatabase should return an empty object when WebSQL is disabled

• Component: DOM
• Status: RESOLVED FIXED

[Sihui Liu]

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.

[Sihui Liu]

Created attachment 371987 [details]
Patch

[Geoffrey Garen]

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.

[Sihui Liu]

Created attachment 372617 [details]
Patch

[EWS Watchlist]

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

[Sihui Liu]

Created attachment 372638 [details]
Patch

[Sihui Liu]

Created attachment 372641 [details]
Patch

[Geoffrey Garen]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Sihui Liu]

Created attachment 372673 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 372673 [details]
Patch for landing

Clearing flags on attachment: 372673

Committed r246707: <https://trac.webkit.org/changeset/246707>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/52013842>

[Ryosuke Niwa]

Ah, this is a very neat trick!

[Oz]

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.

[Geoffrey Garen]

Filed the typeof bug as https://bugs.webkit.org/show_bug.cgi?id=202485.

[Ryosuke Niwa]

Oz, do you know of any popular website that got broken due to the typeof issue?

[Oz]

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

[Oz]

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

[Ryosuke Niwa]

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

[Oz]

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