45354 2010-09-07 18:38:36 -0700 chromium/mac: Fix crash with compositor due to missing current NSGraphicsContext 2010-09-08 06:02:46 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Other OS X 10.5 RESOLVED FIXED P2 Normal --- 1 thakis thakis commit-queue jamesr kbr vangelis oldest_to_newest 275519 0 thakis 2010-09-07 18:38:36 -0700 chromium/mac: Fix crash with compositor due to missing current NSGraphicsContext 275522 1 66823 thakis 2010-09-07 18:40:10 -0700 Created attachment 66823 Patch 275523 2 thakis 2010-09-07 18:41:27 -0700 Patch also removes some duplicated code. 275525 3 thakis 2010-09-07 18:42:03 -0700 This is supposed to fix http://crbug.com/54763 btw 275561 4 66823 kbr 2010-09-07 19:49:14 -0700 Comment on attachment 66823 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=66823&action=prettypatch Looks good to me. Because I've run into the issue that Chromium doesn't define PLATFORM(MAC) I'm surprised that we compile in code in the WebCore/platform/mac directory (LocalCurrentGraphicsContext). Is there any possibility that LocalCurrentGraphicsContext is leaking its newly created NSGraphicsContext (by leaving it as the current context) if there isn't a current NSGraphicsContext going in to the constructor? > WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:46 > + Unneeded newline 275570 5 thakis 2010-09-07 19:57:07 -0700 Thanks for the review! (In reply to comment #4) > (From update of attachment 66823 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=66823&action=prettypatch > > Looks good to me. Because I've run into the issue that Chromium doesn't define PLATFORM(MAC) I'm surprised that we compile in code in the WebCore/platform/mac directory (LocalCurrentGraphicsContext). We generally build files in mac/ subdirectories. See WebCore/WebCore.gypi for a list of all files we look at and then grep WebCore/WebCore.gyp/WebCore.gyp for "exclude" for stuff we exclude from that long list. > Is there any possibility that LocalCurrentGraphicsContext is leaking its newly created NSGraphicsContext (by leaving it as the current context) if there isn't a current NSGraphicsContext going in to the constructor? > > > WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:46 > > + > Unneeded newline Most files I looked at had a newline between system stuff and webkit stuff. 275784 6 66823 commit-queue 2010-09-08 06:02:41 -0700 Comment on attachment 66823 Patch Clearing flags on attachment: 66823 Committed r66975: <http://trac.webkit.org/changeset/66975> 275785 7 commit-queue 2010-09-08 06:02:46 -0700 All reviewed patches have been landed. Closing bug. 66823 2010-09-07 18:40:10 -0700 2010-09-08 06:02:41 -0700 Patch bug-45354-20100907184007.patch text/plain 3357 thakis ZGlmZiAtLWdpdCBhL1dlYkNvcmUvQ2hhbmdlTG9nIGIvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXgg MWY3NGE5MTVkYTgxZGI0NjJiYjllNTE2MTFmZTY0OGU2MTMzMmZmNy4uOGFlZTQ2ZjI3ZTg5ZDk3 YjhkOTc4ODdiMGE0ZWE2OGQ3ZDdhMTU0ZiAxMDA2NDQKLS0tIGEvV2ViQ29yZS9DaGFuZ2VMb2cK KysrIGIvV2ViQ29yZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxMyBAQAorMjAxMC0wOS0wNyAgTmlj byBXZWJlciAgPHRoYWtpc0BjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9C T0RZIChPT1BTISkuCisKKyAgICAgICAgY2hyb21pdW0vbWFjOiBGaXggY3Jhc2ggd2l0aCBjb21w b3NpdG9yIGR1ZSB0byBtaXNzaW5nIGN1cnJlbnQgTlNHcmFwaGljc0NvbnRleHQKKyAgICAgICAg aHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTQ1MzU0CisKKyAgICAgICAg KiBwbGF0Zm9ybS9ncmFwaGljcy9jaHJvbWl1bS9Db250ZW50TGF5ZXJDaHJvbWl1bS5jcHA6Cisg ICAgICAgIChXZWJDb3JlOjpDb250ZW50TGF5ZXJDaHJvbWl1bTo6dXBkYXRlQ29udGVudHMpOiBT ZXQgYSBjdXJyZW50IGxvY2FsIGNvbnRleHQuCisKIDIwMTAtMDktMDcgIFNpbW9uIEZyYXNlciAg PHNpbW9uLmZyYXNlckBhcHBsZS5jb20+CiAKICAgICAgICAgUmV2aWV3ZWQgYnkgQ2hyaXMgTWFy cmluLgpkaWZmIC0tZ2l0IGEvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9jaHJvbWl1bS9Db250 ZW50TGF5ZXJDaHJvbWl1bS5jcHAgYi9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL2Nocm9taXVt L0NvbnRlbnRMYXllckNocm9taXVtLmNwcAppbmRleCA5NzQ5MzNkNDViNzc5Y2M5Mzk0MzE5Y2M0 NmQzZjA2ZGNlOTFiNmJmLi41YzhiODY5M2RhN2RlNjNhMjM5OTk4MjgyZDlhY2ZjYmViY2UxYjgz IDEwMDY0NAotLS0gYS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL2Nocm9taXVtL0NvbnRlbnRM YXllckNocm9taXVtLmNwcAorKysgYi9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL2Nocm9taXVt L0NvbnRlbnRMYXllckNocm9taXVtLmNwcApAQCAtNDIsNiArNDIsOCBAQAogI2luY2x1ZGUgIlBs YXRmb3JtQ29udGV4dFNraWEuaCIKICNpbmNsdWRlICJza2lhL2V4dC9wbGF0Zm9ybV9jYW52YXMu aCIKICNlbGlmIFBMQVRGT1JNKENHKQorI2luY2x1ZGUgIkxvY2FsQ3VycmVudEdyYXBoaWNzQ29u dGV4dC5oIgorCiAjaW5jbHVkZSA8Q29yZUdyYXBoaWNzL0NHQml0bWFwQ29udGV4dC5oPgogI2Vu ZGlmCiAKQEAgLTE1MiwxMiArMTU0LDYgQEAgdm9pZCBDb250ZW50TGF5ZXJDaHJvbWl1bTo6dXBk YXRlQ29udGVudHMoKQogICAgIEludFNpemUgcmVxdWlyZWRUZXh0dXJlU2l6ZTsKICAgICBJbnRT aXplIGJpdG1hcFNpemU7CiAKLSNpZiBQTEFURk9STShTS0lBKQotICAgIGNvbnN0IFNrQml0bWFw KiBza2lhQml0bWFwID0gMDsKLSAgICBPd25QdHI8c2tpYTo6UGxhdGZvcm1DYW52YXM+IGNhbnZh czsKLSAgICBPd25QdHI8UGxhdGZvcm1Db250ZXh0U2tpYT4gc2tpYUNvbnRleHQ7Ci0gICAgT3du UHRyPEdyYXBoaWNzQ29udGV4dD4gZ3JhcGhpY3NDb250ZXh0OwotCiAgICAgcmVxdWlyZWRUZXh0 dXJlU2l6ZSA9IG1fYm91bmRzOwogICAgIEludFJlY3QgYm91bmRzUmVjdChJbnRQb2ludCgwLCAw KSwgbV9ib3VuZHMpOwogCkBAIC0xNzEsNiArMTY3LDEyIEBAIHZvaWQgQ29udGVudExheWVyQ2hy b21pdW06OnVwZGF0ZUNvbnRlbnRzKCkKICAgICAgICAgZGlydHlSZWN0LmludGVyc2VjdChib3Vu ZHNSZWN0KTsKICAgICB9CiAKKyNpZiBQTEFURk9STShTS0lBKQorICAgIGNvbnN0IFNrQml0bWFw KiBza2lhQml0bWFwID0gMDsKKyAgICBPd25QdHI8c2tpYTo6UGxhdGZvcm1DYW52YXM+IGNhbnZh czsKKyAgICBPd25QdHI8UGxhdGZvcm1Db250ZXh0U2tpYT4gc2tpYUNvbnRleHQ7CisgICAgT3du UHRyPEdyYXBoaWNzQ29udGV4dD4gZ3JhcGhpY3NDb250ZXh0OworCiAgICAgY2FudmFzLnNldChu ZXcgc2tpYTo6UGxhdGZvcm1DYW52YXMoZGlydHlSZWN0LndpZHRoKCksIGRpcnR5UmVjdC5oZWln aHQoKSwgZmFsc2UpKTsKICAgICBza2lhQ29udGV4dC5zZXQobmV3IFBsYXRmb3JtQ29udGV4dFNr aWEoY2FudmFzLmdldCgpKSk7CiAKQEAgLTIwMSwxOSArMjAzLDYgQEAgdm9pZCBDb250ZW50TGF5 ZXJDaHJvbWl1bTo6dXBkYXRlQ29udGVudHMoKQogICAgICAgICBiaXRtYXBTaXplID0gSW50U2l6 ZShza2lhQml0bWFwLT53aWR0aCgpLCBza2lhQml0bWFwLT5oZWlnaHQoKSk7CiAgICAgfQogI2Vs aWYgUExBVEZPUk0oQ0cpCi0gICAgcmVxdWlyZWRUZXh0dXJlU2l6ZSA9IG1fYm91bmRzOwotICAg IEludFJlY3QgYm91bmRzUmVjdChJbnRQb2ludCgwLCAwKSwgbV9ib3VuZHMpOwotCi0gICAgLy8g SWYgdGhlIHRleHR1cmUgbmVlZHMgdG8gYmUgcmVhbGxvY2F0ZWQgdGhlbiB3ZSBtdXN0IHJlZHJh dyB0aGUgZW50aXJlCi0gICAgLy8gY29udGVudHMgb2YgdGhlIGxheWVyLgotICAgIGlmIChyZXF1 aXJlZFRleHR1cmVTaXplICE9IG1fYWxsb2NhdGVkVGV4dHVyZVNpemUpCi0gICAgICAgIGRpcnR5 UmVjdCA9IGJvdW5kc1JlY3Q7Ci0gICAgZWxzZSB7Ci0gICAgICAgIC8vIENsaXAgdGhlIGRpcnR5 UmVjdCB0byB0aGUgc2l6ZSBvZiB0aGUgbGF5ZXIgdG8gYXZvaWQgZHJhd2luZyBvdXRzaWRlCi0g ICAgICAgIC8vIHRoZSBib3VuZHMgb2YgdGhlIGJhY2tpbmcgdGV4dHVyZS4KLSAgICAgICAgZGly dHlSZWN0LmludGVyc2VjdChib3VuZHNSZWN0KTsKLSAgICB9Ci0KICAgICBWZWN0b3I8dWludDhf dD4gdGVtcFZlY3RvcjsKICAgICBpbnQgcm93Qnl0ZXMgPSA0ICogZGlydHlSZWN0LndpZHRoKCk7 CiAgICAgdGVtcFZlY3Rvci5yZXNpemUocm93Qnl0ZXMgKiBkaXJ0eVJlY3QuaGVpZ2h0KCkpOwpA QCAtMjI1LDYgKzIxNCw3IEBAIHZvaWQgQ29udGVudExheWVyQ2hyb21pdW06OnVwZGF0ZUNvbnRl bnRzKCkKICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgICAgIGtDR0ltYWdlQWxwaGFQcmVtdWx0aXBsaWVkTGFzdCkpOwogCiAg ICAgR3JhcGhpY3NDb250ZXh0IGdyYXBoaWNzQ29udGV4dChjb250ZXh0Q0cuZ2V0KCkpOworICAg IExvY2FsQ3VycmVudEdyYXBoaWNzQ29udGV4dCBzY29wZWROU0dyYXBoaWNzQ29udGV4dCgmZ3Jh cGhpY3NDb250ZXh0KTsKIAogICAgIC8vIFRyYW5zbGF0ZSB0aGUgZ3JhcGhpY3MgY29udHh0IGlu dG8gdGhlIGNvb3JkaW5hdGUgc3lzdGVtIG9mIHRoZSBkaXJ0eSByZWN0LgogICAgIGdyYXBoaWNz Q29udGV4dC50cmFuc2xhdGUoLWRpcnR5UmVjdC54KCksIC1kaXJ0eVJlY3QueSgpKTsK