67805 2011-09-08 14:28:24 -0700 [Chromium] Crash in WebCore::DatabaseObserver 2011-09-23 15:33:29 -0700 1 1 1 Unclassified WebKit WebKit Misc. 528+ (Nightly build) Other All RESOLVED FIXED P2 Normal --- 1 schenney schenney abarth dglazkov levin michaeln schenney webkit.review.bot oldest_to_newest 464372 0 schenney 2011-09-08 14:28:24 -0700 http://code.google.com/p/chromium/issues/detail?id=88042 WebFrameImpl::fromFrame may return null, and DatabaseObserver::canEstablishDatabase does not check for this, leading to potential crashes. 468070 1 107549 schenney 2011-09-15 14:11:27 -0700 Created attachment 107549 Patch 468179 2 michaeln 2011-09-15 17:34:09 -0700 this looks reasonable to me i think changing the return value at the end to false is reasonable, but i don't know if there are chromium/webkit api users that are depending on the true return value at this time... are their any consumers that don't provide a permissionClient that this change would break? also, i'm noticing that 'true' is the default return value for the allowDatabase method in WebPermissionsClient.h maybe it's safer and more consistent to not change that return value at the end in this crash fixing patch wdyt? 468523 3 schenney 2011-09-16 09:15:12 -0700 I investigated further, looking for cases in the chromium code base where the WebPermissionsClient is set on the WebViewImpl. The only location is in chrome_render_view_observer, and the AllowDatabase setting for that ultimately comes from the browser's cookie settings. So it is certainly plausible that there exist other users of the database code that do not set any permissions client. There is also database related code in WebWorkerClientImpl, and it defaults to true when there is no permissions client (and the permissions client always returns true, as far as I can tell). Given this, and the default of true in the permissions client, I will change the return value. 468530 4 107663 schenney 2011-09-16 09:19:46 -0700 Created attachment 107663 Patch 468790 5 michaeln 2011-09-16 13:43:32 -0700 fwiw lgtm (but i dont have review rights) 469660 6 michaeln 2011-09-19 13:44:34 -0700 adding some webkit reviewers 469683 7 107663 levin 2011-09-19 14:19:52 -0700 Comment on attachment 107663 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=107663&action=review Is it possible to test this? Is it possible for webFrame->viewImpl() to be 0? What about document->frame()? > Source/WebKit/chromium/ChangeLog:10 > + null frame and return false when the frame is null. Investiagted typo: Investiagted 470841 8 schenney 2011-09-21 08:51:46 -0700 I have no idea how to test that this resolves the crash, but I will look into it. At the same time I'll try to figure out why one thing may be null but not another. 471728 9 schenney 2011-09-22 11:19:59 -0700 Is it possible to test this? There was no repro case in the original crash report. It clearly depends on having a script execution context with a valid document (if you have the context you have the document) but an invalid frame. I'll try to repro with that in mind. Is it possible for webFrame->viewImpl() to be 0? webFrame->viewImpl() may be 0 if the frame is 0 or the page for the frame is 0. That means it should be checked too. What about document->frame()? document->frame() is probably the 0 object in this situation, but that does not need to be checked because WebFrameImpl::fromFrame(document->frame()) is happy to return 0 when it gets 0. 472259 10 108481 schenney 2011-09-23 08:45:14 -0700 Created attachment 108481 Patch 472260 11 schenney 2011-09-23 08:52:10 -0700 I have no commit status at this time. Please do the honors for me. 472261 12 108481 levin 2011-09-23 08:53:02 -0700 Comment on attachment 108481 Patch Feel free to mark your patch as cq? if you'd like it marked cq+ :) 472271 13 108481 webkit.review.bot 2011-09-23 09:07:53 -0700 Comment on attachment 108481 Patch Attachment 108481 did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9818156 New failing tests: svg/custom/svg-fonts-word-spacing.html 472347 14 108481 levin 2011-09-23 11:10:28 -0700 Comment on attachment 108481 Patch If at first you don't succeed... that test failure in svg is totally unrelated to this change. 472556 15 108481 webkit.review.bot 2011-09-23 15:33:23 -0700 Comment on attachment 108481 Patch Clearing flags on attachment: 108481 Committed r95871: <http://trac.webkit.org/changeset/95871> 472557 16 webkit.review.bot 2011-09-23 15:33:29 -0700 All reviewed patches have been landed. Closing bug. 107549 2011-09-15 14:11:27 -0700 2011-09-16 09:19:42 -0700 Patch bug-67805-20110915171126.patch text/plain 1826 schenney SW5kZXg6IFNvdXJjZS9XZWJLaXQvY2hyb21pdW0vQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNv dXJjZS9XZWJLaXQvY2hyb21pdW0vQ2hhbmdlTG9nCShyZXZpc2lvbiA5NTIyNikKKysrIFNvdXJj ZS9XZWJLaXQvY2hyb21pdW0vQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTYg QEAKKzIwMTEtMDktMTUgIFN0ZXBoZW4gQ2hlbm5leSAgPHNjaGVubmV5QGNocm9taXVtLm9yZz4K KworICAgICAgICBbQ2hyb21pdW1dIENyYXNoIGluIFdlYkNvcmU6OkRhdGFiYXNlT2JzZXJ2ZXIK KyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTY3ODA1CisK KyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgKiBzcmMvRGF0 YWJhc2VPYnNlcnZlci5jcHA6CisgICAgICAgIChXZWJDb3JlOjpEYXRhYmFzZU9ic2VydmVyOjpj YW5Fc3RhYmxpc2hEYXRhYmFzZSk6IEFkZGVkIGEgY2hlY2sgZm9yIGEKKyAgICAgICAgbnVsbCBm cmFtZSwgYW5kIGNoYW5nZWQgdGhlIGZhbGwtdGhyb3VnaCByZXR1cm4gdmFsdWUgdG8gZmFsc2Uu IFRoaXMKKyAgICAgICAgaW1wbGllcyB0aGF0IGFuIGF0dGVtcHQgdG8gZXN0YWJsaXNoIGEgZGF0 YWJhc2UgZm9yIGEgZnJhbWUgd2l0aCBubworICAgICAgICBwZXJtaXNzaW9uc0NsaWVudCB3aWxs IG5vdyBmYWlsLgorCiAyMDExLTA5LTE1ICBKb24gTGVlICA8am9ubGVlQGFwcGxlLmNvbT4KIAog ICAgICAgICBTdWJtaXR0aW5nIGEgZm9ybSB3aXRoIHRhcmdldD1fYmxhbmsgd29ya3Mgb25seSBv bmNlCkluZGV4OiBTb3VyY2UvV2ViS2l0L2Nocm9taXVtL3NyYy9EYXRhYmFzZU9ic2VydmVyLmNw cAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2ViS2l0L2Nocm9taXVtL3NyYy9EYXRhYmFzZU9ic2Vy dmVyLmNwcAkocmV2aXNpb24gOTUyMTkpCisrKyBTb3VyY2UvV2ViS2l0L2Nocm9taXVtL3NyYy9E YXRhYmFzZU9ic2VydmVyLmNwcAkod29ya2luZyBjb3B5KQpAQCAtMTU4LDYgKzE1OCw4IEBAIGJv b2wgRGF0YWJhc2VPYnNlcnZlcjo6Y2FuRXN0YWJsaXNoRGF0YWIKICAgICBpZiAoc2NyaXB0RXhl Y3V0aW9uQ29udGV4dC0+aXNEb2N1bWVudCgpKSB7CiAgICAgICAgIERvY3VtZW50KiBkb2N1bWVu dCA9IHN0YXRpY19jYXN0PERvY3VtZW50Kj4oc2NyaXB0RXhlY3V0aW9uQ29udGV4dCk7CiAgICAg ICAgIFdlYkZyYW1lSW1wbCogd2ViRnJhbWUgPSBXZWJGcmFtZUltcGw6OmZyb21GcmFtZShkb2N1 bWVudC0+ZnJhbWUoKSk7CisgICAgICAgIGlmICghd2ViRnJhbWUpCisgICAgICAgICAgICByZXR1 cm4gZmFsc2U7CiAgICAgICAgIFdlYlZpZXdJbXBsKiB3ZWJWaWV3ID0gd2ViRnJhbWUtPnZpZXdJ bXBsKCk7CiAgICAgICAgIGlmICh3ZWJWaWV3LT5wZXJtaXNzaW9uQ2xpZW50KCkpCiAgICAgICAg ICAgICByZXR1cm4gd2ViVmlldy0+cGVybWlzc2lvbkNsaWVudCgpLT5hbGxvd0RhdGFiYXNlKHdl YkZyYW1lLCBuYW1lLCBkaXNwbGF5TmFtZSwgZXN0aW1hdGVkU2l6ZSk7CkBAIC0xNzIsNyArMTc0 LDcgQEAgYm9vbCBEYXRhYmFzZU9ic2VydmVyOjpjYW5Fc3RhYmxpc2hEYXRhYgogI2VuZGlmCiAg ICAgfQogCi0gICAgcmV0dXJuIHRydWU7CisgICAgcmV0dXJuIGZhbHNlOwogfQogCiB2b2lkIERh dGFiYXNlT2JzZXJ2ZXI6OmRhdGFiYXNlT3BlbmVkKEFic3RyYWN0RGF0YWJhc2UqIGRhdGFiYXNl KQo= 107663 2011-09-16 09:19:46 -0700 2011-09-23 08:45:07 -0700 Patch bug-67805-20110916121944.patch text/plain 1691 schenney SW5kZXg6IFNvdXJjZS9XZWJLaXQvY2hyb21pdW0vQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNv dXJjZS9XZWJLaXQvY2hyb21pdW0vQ2hhbmdlTG9nCShyZXZpc2lvbiA5NTIyNikKKysrIFNvdXJj ZS9XZWJLaXQvY2hyb21pdW0vQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTcg QEAKKzIwMTEtMDktMTUgIFN0ZXBoZW4gQ2hlbm5leSAgPHNjaGVubmV5QGNocm9taXVtLm9yZz4K KworICAgICAgICBbQ2hyb21pdW1dIENyYXNoIGluIFdlYkNvcmU6OkRhdGFiYXNlT2JzZXJ2ZXIK KyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTY3ODA1CisK KyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgKiBzcmMvRGF0 YWJhc2VPYnNlcnZlci5jcHA6CisgICAgICAgIChXZWJDb3JlOjpEYXRhYmFzZU9ic2VydmVyOjpj YW5Fc3RhYmxpc2hEYXRhYmFzZSk6IEFkZGVkIGEgY2hlY2sgZm9yIGEKKyAgICAgICAgbnVsbCBm cmFtZSBhbmQgcmV0dXJuIGZhbHNlIHdoZW4gdGhlIGZyYW1lIGlzIG51bGwuIEludmVzdGlhZ3Rl ZAorICAgICAgICBjaGFuZ2luZyB0aGUgZmFsbC10aHJvdWdoIHJldHVybiB2YWx1ZSB0byBmYWxz ZSBidXQgZGVjaWRlZCBhZ2FpbnN0CisgICAgICAgIGl0IGdpdmVuIHRoZSB3YXkgdGhlIGNvZGUg aXMgdXNlZCBhbmQgZXhpc3RpbmcgZGVmYXVsdCB2YWx1ZXMgZm9yCisgICAgICAgIHJlbGF0ZWQg Y29kZS4KKwogMjAxMS0wOS0xNSAgSm9uIExlZSAgPGpvbmxlZUBhcHBsZS5jb20+CiAKICAgICAg ICAgU3VibWl0dGluZyBhIGZvcm0gd2l0aCB0YXJnZXQ9X2JsYW5rIHdvcmtzIG9ubHkgb25jZQpJ bmRleDogU291cmNlL1dlYktpdC9jaHJvbWl1bS9zcmMvRGF0YWJhc2VPYnNlcnZlci5jcHAKPT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PQotLS0gU291cmNlL1dlYktpdC9jaHJvbWl1bS9zcmMvRGF0YWJhc2VPYnNlcnZlci5j cHAJKHJldmlzaW9uIDk1MjE5KQorKysgU291cmNlL1dlYktpdC9jaHJvbWl1bS9zcmMvRGF0YWJh c2VPYnNlcnZlci5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTE1OCw2ICsxNTgsOCBAQCBib29sIERh dGFiYXNlT2JzZXJ2ZXI6OmNhbkVzdGFibGlzaERhdGFiCiAgICAgaWYgKHNjcmlwdEV4ZWN1dGlv bkNvbnRleHQtPmlzRG9jdW1lbnQoKSkgewogICAgICAgICBEb2N1bWVudCogZG9jdW1lbnQgPSBz dGF0aWNfY2FzdDxEb2N1bWVudCo+KHNjcmlwdEV4ZWN1dGlvbkNvbnRleHQpOwogICAgICAgICBX ZWJGcmFtZUltcGwqIHdlYkZyYW1lID0gV2ViRnJhbWVJbXBsOjpmcm9tRnJhbWUoZG9jdW1lbnQt PmZyYW1lKCkpOworICAgICAgICBpZiAoIXdlYkZyYW1lKQorICAgICAgICAgICAgcmV0dXJuIGZh bHNlOwogICAgICAgICBXZWJWaWV3SW1wbCogd2ViVmlldyA9IHdlYkZyYW1lLT52aWV3SW1wbCgp OwogICAgICAgICBpZiAod2ViVmlldy0+cGVybWlzc2lvbkNsaWVudCgpKQogICAgICAgICAgICAg cmV0dXJuIHdlYlZpZXctPnBlcm1pc3Npb25DbGllbnQoKS0+YWxsb3dEYXRhYmFzZSh3ZWJGcmFt ZSwgbmFtZSwgZGlzcGxheU5hbWUsIGVzdGltYXRlZFNpemUpOwo= 108481 2011-09-23 08:45:14 -0700 2011-09-23 15:33:23 -0700 Patch bug-67805-20110923114512.patch text/plain 1909 schenney SW5kZXg6IFNvdXJjZS9XZWJLaXQvY2hyb21pdW0vQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNv dXJjZS9XZWJLaXQvY2hyb21pdW0vQ2hhbmdlTG9nCShyZXZpc2lvbiA5NTgwNikKKysrIFNvdXJj ZS9XZWJLaXQvY2hyb21pdW0vQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTkg QEAKKzIwMTEtMDktMjMgIFN0ZXBoZW4gQ2hlbm5leSAgPHNjaGVubmV5QGNocm9taXVtLm9yZz4K KworICAgICAgICBbQ2hyb21pdW1dIENyYXNoIGluIFdlYkNvcmU6OkRhdGFiYXNlT2JzZXJ2ZXIK KyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTY3ODA1CisK KyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgKiBzcmMvRGF0 YWJhc2VPYnNlcnZlci5jcHA6CisgICAgICAgIChXZWJDb3JlOjpEYXRhYmFzZU9ic2VydmVyOjpj YW5Fc3RhYmxpc2hEYXRhYmFzZSk6IEFkZGVkIGEgY2hlY2sgZm9yIGEKKyAgICAgICAgbnVsbCBm cmFtZSBvciBwYWdlLCBhbmQgcmV0dXJuIGZhbHNlIGlmIG51bGwuIEludmVzdGlnYXRlZAorICAg ICAgICBjaGFuZ2luZyB0aGUgZmFsbC10aHJvdWdoIHJldHVybiB2YWx1ZSB0byBmYWxzZSBidXQg ZGVjaWRlZCBhZ2FpbnN0CisgICAgICAgIGl0IGdpdmVuIHRoZSB3YXkgdGhlIGNvZGUgaXMgdXNl ZCBhbmQgZXhpc3RpbmcgZGVmYXVsdCB2YWx1ZXMgZm9yCisgICAgICAgIHJlbGF0ZWQgY29kZS4g UmVwcm9kdWN0aW9uIGFuZCB0ZXN0aW5nIGRlcGVuZHMgb24gaGF2aW5nIGEgZG9jdW1lbnQKKyAg ICAgICAgd2l0aCBubyBmcmFtZSBvciBubyBwYWdlLCB3aGljaCBzb21ldGltZXMgaGFwcGVucyBp biBwcmFjdGljZSBidXQgaXMKKyAgICAgICAgaGFyZCB0byBjb25zdHJ1Y3QgZXhwbGljaXRseS4K KwogMjAxMS0wOS0yMiAgU2hlcmlmZiBCb3QgIDx3ZWJraXQucmV2aWV3LmJvdEBnbWFpbC5jb20+ CiAKICAgICAgICAgVW5yZXZpZXdlZC4gIFJvbGxlZCBERVBTLgpJbmRleDogU291cmNlL1dlYktp dC9jaHJvbWl1bS9zcmMvRGF0YWJhc2VPYnNlcnZlci5jcHAKPT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNl L1dlYktpdC9jaHJvbWl1bS9zcmMvRGF0YWJhc2VPYnNlcnZlci5jcHAJKHJldmlzaW9uIDk1ODA2 KQorKysgU291cmNlL1dlYktpdC9jaHJvbWl1bS9zcmMvRGF0YWJhc2VPYnNlcnZlci5jcHAJKHdv cmtpbmcgY29weSkKQEAgLTE1OCw3ICsxNTgsMTEgQEAgYm9vbCBEYXRhYmFzZU9ic2VydmVyOjpj YW5Fc3RhYmxpc2hEYXRhYgogICAgIGlmIChzY3JpcHRFeGVjdXRpb25Db250ZXh0LT5pc0RvY3Vt ZW50KCkpIHsKICAgICAgICAgRG9jdW1lbnQqIGRvY3VtZW50ID0gc3RhdGljX2Nhc3Q8RG9jdW1l bnQqPihzY3JpcHRFeGVjdXRpb25Db250ZXh0KTsKICAgICAgICAgV2ViRnJhbWVJbXBsKiB3ZWJG cmFtZSA9IFdlYkZyYW1lSW1wbDo6ZnJvbUZyYW1lKGRvY3VtZW50LT5mcmFtZSgpKTsKKyAgICAg ICAgaWYgKCF3ZWJGcmFtZSkKKyAgICAgICAgICAgIHJldHVybiBmYWxzZTsKICAgICAgICAgV2Vi Vmlld0ltcGwqIHdlYlZpZXcgPSB3ZWJGcmFtZS0+dmlld0ltcGwoKTsKKyAgICAgICAgaWYgKCF3 ZWJWaWV3KQorICAgICAgICAgICAgcmV0dXJuIGZhbHNlOwogICAgICAgICBpZiAod2ViVmlldy0+ cGVybWlzc2lvbkNsaWVudCgpKQogICAgICAgICAgICAgcmV0dXJuIHdlYlZpZXctPnBlcm1pc3Np b25DbGllbnQoKS0+YWxsb3dEYXRhYmFzZSh3ZWJGcmFtZSwgbmFtZSwgZGlzcGxheU5hbWUsIGVz dGltYXRlZFNpemUpOwogICAgIH0gZWxzZSB7Cg==