209160 2020-03-16 17:15:12 -0700 REGRESSION(r254856) Add exception for window.openDatabase to not masquerade as undefined in currently shipping Jesus Calling Devotional app 2020-04-24 10:14:44 -0700 1 1 1 Unclassified WebKit New Bugs WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 achristensen achristensen benjamin cdumez cmarcelo ews-watchlist ggaren rniwa sihui_liu webkit-bug-importer oldest_to_newest 1630576 0 achristensen 2020-03-16 17:15:12 -0700 REGRESSION(r254856) Add exception for window.openDatabase to not masquerade as undefined in currently shipping Jesus Calling Devotional app 1630578 1 393713 achristensen 2020-03-16 17:19:25 -0700 Created attachment 393713 patch 1630654 2 393713 rniwa 2020-03-17 00:06:12 -0700 Comment on attachment 393713 patch View in context: https://bugs.webkit.org/attachment.cgi?id=393713&action=review > Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:647 > - if (RuntimeEnabledFeatures::sharedFeatures().webSQLEnabled()) > + if (RuntimeEnabledFeatures::sharedFeatures().webSQLEnabled() || needsWebSQLException) This will enable WebSQL. Is that what we want? Don't we want to make openDatabase be undefined instead? 1630766 3 393713 ggaren 2020-03-17 10:06:17 -0700 Comment on attachment 393713 patch View in context: https://bugs.webkit.org/attachment.cgi?id=393713&action=review >> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:647 >> + if (RuntimeEnabledFeatures::sharedFeatures().webSQLEnabled() || needsWebSQLException) > > This will enable WebSQL. Is that what we want? > Don't we want to make openDatabase be undefined instead? The mechanical behavior implemented here makes window.openDatabase present and defined, and then when you call it, it throws an exception. If this is truly the behavior required to achieve compatibility, then I'd suggest renaming "needsWebSQLException" to something else, since the meaning of "exception" is not clear here. Maybe "webSQLShouldBeDefinedEvenWhenDisabled". But it's also a mystery that inviting the app to throw an exception somehow fixes the app. Does your debugging shed any light on that requirement? 1630772 4 achristensen 2020-03-17 10:23:00 -0700 As described in rdar://problem/60297073, they are using lines like this to detect WebKit: typeof openDatabase !== 'undefined' Changing the type of window.openDatabase to not undefined makes the WebKit detection succeed, which makes the iOS app work. r=you with the rename to webSQLShouldBeDefinedEvenWhenDisabled before landing? 1630773 5 achristensen 2020-03-17 10:23:51 -0700 openDatabaseShouldBeDefinedEvenWhenDisabled 1630778 6 393713 ggaren 2020-03-17 10:34:05 -0700 Comment on attachment 393713 patch OK, r=me with that rename Thanks for the explanation! 1630780 7 sihui_liu 2020-03-17 10:40:50 -0700 I don't think we want to allow them to do this kind of detection. If we have to make the quirk, it may be better just to enable WebSQL, which makes our code easier to understand. We already have 1. openDatabase undefined but callable 2. openDatabase defined and callable now we add 3. openDatabase defined but not callable 1630785 8 ggaren 2020-03-17 10:58:12 -0700 I agree that this is a weird behavior. (Bear in mind that this patch does not support this weird behavior indefinitely; only until the next time the app is updated.) We could just enable WebSQL for this app instead. The upside is that we would maintain only two states, instead of adding a third. The downside is that we would have to maintain our WebSQL implementation in-tree longer. (I believe that we have no other known blockers to removing WebSQL from the tree.) Do you think it is worth it to keep the WebSQL implementation in-tree longer? 1630794 9 sihui_liu 2020-03-17 11:13:13 -0700 (In reply to Geoffrey Garen from comment #8) > I agree that this is a weird behavior. (Bear in mind that this patch does > not support this weird behavior indefinitely; only until the next time the > app is updated.) > > We could just enable WebSQL for this app instead. The upside is that we > would maintain only two states, instead of adding a third. The downside is > that we would have to maintain our WebSQL implementation in-tree longer. (I > believe that we have no other known blockers to removing WebSQL from the > tree.) > > Do you think it is worth it to keep the WebSQL implementation in-tree longer? We don't need to keep the implementation once we are sure there is no use of WebSQL other than using openDatabase for weird detections. We may just keep openDatabase for those detections and ask the sites to stop doing this. 1630808 10 achristensen 2020-03-17 11:30:26 -0700 I think this is definitely the way to go in this case because we don't want to have to wait for a third party application to update before removing the WebSQL implementation. We will just have to wait before removing this quirk. 1630809 11 achristensen 2020-03-17 11:31:38 -0700 http://trac.webkit.org/r258564 1630811 12 webkit-bug-importer 2020-03-17 11:32:13 -0700 <rdar://problem/60547018> 1630819 13 ggaren 2020-03-17 11:40:41 -0700 > > Do you think it is worth it to keep the WebSQL implementation in-tree longer? > > We don't need to keep the implementation once we are sure there is no use of > WebSQL other than using openDatabase for weird detections. We may just keep > openDatabase for those detections and ask the sites to stop doing this. There's a part of this proposal I'm unclear on: If we change the code to enable WebSQL in this app, how will the code record the fact that the app could actually function correctly without WebSQL enabled? (And how will we and the bug originator verify that fact through testing?) We could propose adding a comment in the code to that effect, but I think that would still add the third weird behavior as a mental tax in the code, only in a way that isn't tested, which might be worse. 1644842 14 achristensen 2020-04-23 10:30:07 -0700 see https://bugs.webkit.org/show_bug.cgi?id=210889 1644854 15 cdumez 2020-04-23 10:50:26 -0700 (In reply to Geoffrey Garen from comment #13) > > > Do you think it is worth it to keep the WebSQL implementation in-tree longer? > > > > We don't need to keep the implementation once we are sure there is no use of > > WebSQL other than using openDatabase for weird detections. We may just keep > > openDatabase for those detections and ask the sites to stop doing this. > > There's a part of this proposal I'm unclear on: If we change the code to > enable WebSQL in this app, how will the code record the fact that the app > could actually function correctly without WebSQL enabled? (And how will we > and the bug originator verify that fact through testing?) > > We could propose adding a comment in the code to that effect, but I think > that would still add the third weird behavior as a mental tax in the code, > only in a way that isn't tested, which might be worse. This patch uses a linked on after check. Therefore, when developers of their app rebuild with new SDK, they will have to make sure their app works without WebSQL. However, with the partial WebSQL enablement approach in this patch, we risk the chance of the app still being broken in subtle ways with the old SDK :( 1644857 16 cdumez 2020-04-23 10:52:07 -0700 (In reply to Chris Dumez from comment #15) > (In reply to Geoffrey Garen from comment #13) > > > > Do you think it is worth it to keep the WebSQL implementation in-tree longer? > > > > > > We don't need to keep the implementation once we are sure there is no use of > > > WebSQL other than using openDatabase for weird detections. We may just keep > > > openDatabase for those detections and ask the sites to stop doing this. > > > > There's a part of this proposal I'm unclear on: If we change the code to > > enable WebSQL in this app, how will the code record the fact that the app > > could actually function correctly without WebSQL enabled? (And how will we > > and the bug originator verify that fact through testing?) > > > > We could propose adding a comment in the code to that effect, but I think > > that would still add the third weird behavior as a mental tax in the code, > > only in a way that isn't tested, which might be worse. > > This patch uses a linked on after check. Therefore, when developers of their > app rebuild with new SDK, they will have to make sure their app works > without WebSQL. > > However, with the partial WebSQL enablement approach in this patch, we risk > the chance of the app still being broken in subtle ways with the old SDK :( Maybe I am misunderstanding this patch and it does not actually enable WebSQL, merely helps the app with feature detection so that the app fallback nicely to something else? 1645243 17 achristensen 2020-04-24 10:14:44 -0700 This patch just made the type of openDatabase different to enable the app to open successfully, but it was insufficient because after opening there was some actual use of webSQL functionality in the app, which is why we need bug 210889 393713 2020-03-16 17:19:25 -0700 2020-03-17 10:34:05 -0700 patch patch.diff text/plain 5004 achristensen SW5kZXg6IFNvdXJjZS9XVEYvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XVEYvQ2hh bmdlTG9nCShyZXZpc2lvbiAyNTg0NjApCisrKyBTb3VyY2UvV1RGL0NoYW5nZUxvZwkod29ya2lu ZyBjb3B5KQpAQCAtMSwzICsxLDEzIEBACisyMDIwLTAzLTE2ICBBbGV4IENocmlzdGVuc2VuICA8 YWNocmlzdGVuc2VuQHdlYmtpdC5vcmc+CisKKyAgICAgICAgUkVHUkVTU0lPTihyMjU0ODU2KSBB ZGQgZXhjZXB0aW9uIGZvciB3aW5kb3cub3BlbkRhdGFiYXNlIHRvIG5vdCBtYXNxdWVyYWRlIGFz IHVuZGVmaW5lZCBpbiBjdXJyZW50bHkgc2hpcHBpbmcgSmVzdXMgQ2FsbGluZyBEZXZvdGlvbmFs IGFwcAorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MjA5 MTYwCisgICAgICAgIDxyZGFyOi8vcHJvYmxlbS82MDI5NzA3Mz4KKworICAgICAgICBSZXZpZXdl ZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICAqIHd0Zi9zcGkvZGFyd2luL2R5bGRTUEku aDoKKwogMjAyMC0wMy0xMyAgTXlsZXMgQy4gTWF4ZmllbGQgIDxtbWF4ZmllbGRAYXBwbGUuY29t PgogCiAgICAgICAgIFtDb2NvYV0gUHVzaCBhcHBsaWNhdGlvblNES1ZlcnNpb24oKSBkb3duIGZy b20gV2ViQ29yZSBpbnRvIFdURgpJbmRleDogU291cmNlL1dURi93dGYvc3BpL2Rhcndpbi9keWxk U1BJLmgKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dURi93dGYvc3BpL2Rhcndpbi9keWxkU1BJLmgJ KHJldmlzaW9uIDI1ODQ2MCkKKysrIFNvdXJjZS9XVEYvd3RmL3NwaS9kYXJ3aW4vZHlsZFNQSS5o CSh3b3JraW5nIGNvcHkpCkBAIC04NSw2ICs4NSw3IEBACiAjZGVmaW5lIERZTERfSU9TX1ZFUlNJ T05fMTFfMCAweDAwMEIwMDAwCiAjZGVmaW5lIERZTERfSU9TX1ZFUlNJT05fMTFfMyAweDAwMEIw MzAwCiAjZGVmaW5lIERZTERfSU9TX1ZFUlNJT05fMTJfMCAweDAwMEMwMDAwCisjZGVmaW5lIERZ TERfSU9TX1ZFUlNJT05fMTJfMiAweDAwMEMwMjAwCiAjZGVmaW5lIERZTERfSU9TX1ZFUlNJT05f MTNfMCAweDAwMEQwMDAwCiAjZGVmaW5lIERZTERfSU9TX1ZFUlNJT05fMTNfMiAweDAwMEQwMjAw CiAjZGVmaW5lIERZTERfSU9TX1ZFUlNJT05fMTNfNCAweDAwMEQwNDAwCkluZGV4OiBTb3VyY2Uv V2ViQ29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiAyNTg0NjApCisrKyBTb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cJKHdvcmtpbmcg Y29weSkKQEAgLTEsMyArMSwyMCBAQAorMjAyMC0wMy0xNiAgQWxleCBDaHJpc3RlbnNlbiAgPGFj aHJpc3RlbnNlbkB3ZWJraXQub3JnPgorCisgICAgICAgIFJFR1JFU1NJT04ocjI1NDg1NikgQWRk IGV4Y2VwdGlvbiBmb3Igd2luZG93Lm9wZW5EYXRhYmFzZSB0byBub3QgbWFzcXVlcmFkZSBhcyB1 bmRlZmluZWQgaW4gY3VycmVudGx5IHNoaXBwaW5nIEplc3VzIENhbGxpbmcgRGV2b3Rpb25hbCBh cHAKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTIwOTE2 MAorICAgICAgICA8cmRhcjovL3Byb2JsZW0vNjAyOTcwNzM+CisKKyAgICAgICAgUmV2aWV3ZWQg YnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgTWFudWFsbHkgdmVyaWZpZWQgdGhpcyBmaXhl cyB0aGUgYXBwLCB3aGljaCBjb21wYXJlcyB0eXBlb2Ygb3BlbkRhdGFiYXNlIHdpdGggJ3VuZGVm aW5lZCcKKyAgICAgICAgR29pbmcgZm9yd2FyZCwgd2UgaW50ZW5kIHRvIGNvbXBsZXRlbHkgcmVt b3ZlIFdlYlNRTCwgc28gdGhpcyBpcyBhIHRlbXBvcmFyeSBleGNlcHRpb24gdG8gb3VyIHJlbW92 YWwgc3RyYXRlZ3kuCisKKyAgICAgICAgKiBiaW5kaW5ncy9qcy9KU0RPTVdpbmRvd0N1c3RvbS5j cHA6CisgICAgICAgIChXZWJDb3JlOjpKU0RPTVdpbmRvdzo6b3BlbkRhdGFiYXNlIGNvbnN0KToK KyAgICAgICAgKiBwbGF0Zm9ybS9SdW50aW1lQXBwbGljYXRpb25DaGVja3MuaDoKKyAgICAgICAg KiBwbGF0Zm9ybS9jb2NvYS9SdW50aW1lQXBwbGljYXRpb25DaGVja3NDb2NvYS5tbToKKyAgICAg ICAgKFdlYkNvcmU6OklPU0FwcGxpY2F0aW9uOjppc0plc3VzQ2FsbGluZyk6CisKIDIwMjAtMDMt MTMgIFNhaWQgQWJvdS1IYWxsYXdhICA8c2FpZEBhcHBsZS5jb20+CiAKICAgICAgICAgU1ZHTWF0 cml4IHNob3VsZCBoYXZlIHRoZSBhY2Nlc3MgcmlnaHQgb2YgaXRzIG93bmVyIFNWR1RyYW5zZm9y bSBhbHdheXMKSW5kZXg6IFNvdXJjZS9XZWJDb3JlL2JpbmRpbmdzL2pzL0pTRE9NV2luZG93Q3Vz dG9tLmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2ViQ29yZS9iaW5kaW5ncy9qcy9KU0RPTVdp bmRvd0N1c3RvbS5jcHAJKHJldmlzaW9uIDI1ODQ2MCkKKysrIFNvdXJjZS9XZWJDb3JlL2JpbmRp bmdzL2pzL0pTRE9NV2luZG93Q3VzdG9tLmNwcAkod29ya2luZyBjb3B5KQpAQCAtNTksNiArNTks MTAgQEAKICNpbmNsdWRlICJKU1dlYktpdE5hbWVzcGFjZS5oIgogI2VuZGlmCiAKKyNpZiBQTEFU Rk9STShJT1MpCisjaW5jbHVkZSAiUnVudGltZUFwcGxpY2F0aW9uQ2hlY2tzLmgiCisjaW5jbHVk ZSA8d3RmL2NvY29hL1J1bnRpbWVBcHBsaWNhdGlvbkNoZWNrc0NvY29hLmg+CisjZW5kaWYKIAog bmFtZXNwYWNlIFdlYkNvcmUgewogdXNpbmcgbmFtZXNwYWNlIEpTQzsKQEAgLTYzMiw5ICs2MzYs MTUgQEAKIAogSlNWYWx1ZSBKU0RPTVdpbmRvdzo6b3BlbkRhdGFiYXNlKEpTQzo6SlNHbG9iYWxP YmplY3QmIGxleGljYWxHbG9iYWxPYmplY3QpIGNvbnN0CiB7CisjaWYgUExBVEZPUk0oSU9TKQor ICAgIHN0YXRpYyBjb25zdCBib29sIG5lZWRzV2ViU1FMRXhjZXB0aW9uID0gSU9TQXBwbGljYXRp b246OmlzSmVzdXNDYWxsaW5nKCkgJiYgYXBwbGljYXRpb25TREtWZXJzaW9uKCkgPD0gRFlMRF9J T1NfVkVSU0lPTl8xMl8yOworI2Vsc2UKKyAgICBjb25zdGV4cHIgYm9vbCBuZWVkc1dlYlNRTEV4 Y2VwdGlvbiA9IGZhbHNlOworI2VuZGlmCisKICAgICBWTSYgdm0gPSBsZXhpY2FsR2xvYmFsT2Jq ZWN0LnZtKCk7CiAgICAgU3RyaW5nSW1wbCogbmFtZSA9IFByb3BlcnR5TmFtZShzdGF0aWNfY2Fz dDxKU1ZNQ2xpZW50RGF0YSo+KHZtLmNsaWVudERhdGEpLT5idWlsdGluTmFtZXMoKS5vcGVuRGF0 YWJhc2VQdWJsaWNOYW1lKCkpLnB1YmxpY05hbWUoKTsKLSAgICBpZiAoUnVudGltZUVuYWJsZWRG ZWF0dXJlczo6c2hhcmVkRmVhdHVyZXMoKS53ZWJTUUxFbmFibGVkKCkpCisgICAgaWYgKFJ1bnRp bWVFbmFibGVkRmVhdHVyZXM6OnNoYXJlZEZlYXR1cmVzKCkud2ViU1FMRW5hYmxlZCgpIHx8IG5l ZWRzV2ViU1FMRXhjZXB0aW9uKQogICAgICAgICByZXR1cm4gSlNGdW5jdGlvbjo6Y3JlYXRlKHZt LCAmbGV4aWNhbEdsb2JhbE9iamVjdCwgNCwgbmFtZSwganNET01XaW5kb3dJbnN0YW5jZUZ1bmN0 aW9uT3BlbkRhdGFiYXNlLCBOb0ludHJpbnNpYyk7CiAKICAgICByZXR1cm4gSlNGdW5jdGlvbjo6 Y3JlYXRlRnVuY3Rpb25UaGF0TWFzcXVlcmFkZXNBc1VuZGVmaW5lZCh2bSwgJmxleGljYWxHbG9i YWxPYmplY3QsIDQsIG5hbWUsIGpzRE9NV2luZG93SW5zdGFuY2VGdW5jdGlvbk9wZW5EYXRhYmFz ZSwgTm9JbnRyaW5zaWMpOwpJbmRleDogU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vUnVudGltZUFw cGxpY2F0aW9uQ2hlY2tzLmgKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dlYkNvcmUvcGxhdGZvcm0v UnVudGltZUFwcGxpY2F0aW9uQ2hlY2tzLmgJKHJldmlzaW9uIDI1ODQ2MCkKKysrIFNvdXJjZS9X ZWJDb3JlL3BsYXRmb3JtL1J1bnRpbWVBcHBsaWNhdGlvbkNoZWNrcy5oCSh3b3JraW5nIGNvcHkp CkBAIC04Niw2ICs4Niw3IEBACiBXRUJDT1JFX0VYUE9SVCBib29sIGlzRHVtcFJlbmRlclRyZWUo KTsKIFdFQkNPUkVfRVhQT1JUIGJvb2wgaXNNaW5pQnJvd3NlcigpOwogYm9vbCBpc01vYmlsZVN0 b3JlKCk7Citib29sIGlzSmVzdXNDYWxsaW5nKCk7CiBib29sIGlzU3ByaW5nQm9hcmQoKTsKIFdF QkNPUkVfRVhQT1JUIGJvb2wgaXNXZWJQcm9jZXNzKCk7CiBXRUJDT1JFX0VYUE9SVCBib29sIGlz SUJvb2tzKCk7CkluZGV4OiBTb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9jb2NvYS9SdW50aW1lQXBw bGljYXRpb25DaGVja3NDb2NvYS5tbQo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2ViQ29yZS9wbGF0 Zm9ybS9jb2NvYS9SdW50aW1lQXBwbGljYXRpb25DaGVja3NDb2NvYS5tbQkocmV2aXNpb24gMjU4 NDYwKQorKysgU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vY29jb2EvUnVudGltZUFwcGxpY2F0aW9u Q2hlY2tzQ29jb2EubW0JKHdvcmtpbmcgY29weSkKQEAgLTI0OSw2ICsyNDksMTIgQEAKICAgICBy ZXR1cm4gaXNEdW1wUmVuZGVyVHJlZTsKIH0KIAorYm9vbCBJT1NBcHBsaWNhdGlvbjo6aXNKZXN1 c0NhbGxpbmcoKQoreworICAgIHN0YXRpYyBib29sIGlzSmVzdXNDYWxsaW5nID0gYXBwbGljYXRp b25CdW5kbGVJc0VxdWFsVG8oImNvbS50aG9tYXNuZWxzb24uamVzdXNjYWxsaW5nIl9zKTsKKyAg ICByZXR1cm4gaXNKZXN1c0NhbGxpbmc7Cit9CisKIGJvb2wgSU9TQXBwbGljYXRpb246OmlzTW9i aWxlU3RvcmUoKQogewogICAgIHN0YXRpYyBib29sIGlzTW9iaWxlU3RvcmUgPSBhcHBsaWNhdGlv bkJ1bmRsZUlzRXF1YWxUbygiY29tLmFwcGxlLk1vYmlsZVN0b3JlIl9zKTsK