| Summary: | REGRESSION(r254856) Add exception for window.openDatabase to not masquerade as undefined in currently shipping Jesus Calling Devotional app | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Alex Christensen <achristensen> | ||||
| Component: | New Bugs | Assignee: | Alex Christensen <achristensen> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | benjamin, cdumez, cmarcelo, ews-watchlist, ggaren, rniwa, sihui_liu, webkit-bug-importer | ||||
| Priority: | P2 | Keywords: | InRadar | ||||
| Version: | WebKit Nightly Build | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Attachments: |
|
||||||
|
Description
Alex Christensen
2020-03-16 17:15:12 PDT
Created attachment 393713 [details]
patch
Comment on attachment 393713 [details] 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? Comment on attachment 393713 [details] 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? 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? openDatabaseShouldBeDefinedEvenWhenDisabled Comment on attachment 393713 [details]
patch
OK, r=me with that rename
Thanks for the explanation!
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 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? (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. 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. > > 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.
(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 :( (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? 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 |