156972 2016-04-25 04:49:42 -0700 [GTK] Crashes if DISPLAY is unset 2016-04-25 13:47:47 -0700 1 1 1 Unclassified WebKit WebKitGTK Other Unspecified Unspecified RESOLVED FIXED http://bugs.debian.org/803104 https://bugzilla.gnome.org/show_bug.cgi?id=757061 P2 Normal --- 1 berto webkit-unassigned bugs-noreply cgarcia kapouer mcatanzaro oldest_to_newest 1186903 0 berto 2016-04-25 04:49:42 -0700 This happens when the gtk-doc scanner is linked against webkit2gtk: gtkdoc-scangobj $scanobj_options --module=libyelp (process:14998): Gtk-CRITICAL **: gtk_icon_theme_get_for_screen: assertion 'GDK_IS_SCREEN (screen)' failed ** (process:14998): WARNING **: Unable to connect to dbus: Cannot autolaunch D-Bus without X11 $DISPLAY Segmentation fault The segfault comes from webkitgtk: (gdb) bt #0 XCloseDisplay (dpy=0x0) at ../../src/ClDisplay.c:51 #1 0x00007ffff4eab831 in WebCore::PlatformDisplayX11::~PlatformDisplayX11 (this=0x7fffe2dfc028, __in_chrg=<optimized out>) at /build/webkit2gtk-ea7lQt/webkit2gtk-2.12.1/Source/WebCore/platform/graphics/x11/PlatformDisplayX11.cpp:53 The problem seems to be that m_display is initialized in the constructor of PlatformDisplayX11, but it's assumed not to be NULL. 1186904 1 277239 berto 2016-04-25 04:53:58 -0700 Created attachment 277239 Patch 1186908 2 277239 cgarcia 2016-04-25 05:31:11 -0700 Comment on attachment 277239 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=277239&action=review > Source/WebCore/platform/graphics/x11/PlatformDisplayX11.cpp:52 > - if (m_ownedDisplay) > + if (m_ownedDisplay && m_display) what about initializing m_ownedDisplay to false if display is NULL? we can't own a display that doesn't exist. 1186928 3 277247 berto 2016-04-25 07:37:46 -0700 Created attachment 277247 Patch Ok, I chose to initialize m_ownedDisplay in the body of the constructor to make sure that m_display is set first. Otherwise we'd have to rely on the order in which m_display and m_ownedDisplay are declared in the class definition. 1186934 4 cgarcia 2016-04-25 08:08:49 -0700 (In reply to comment #3) > Created attachment 277247 [details] > Patch > > Ok, I chose to initialize m_ownedDisplay in the body of the constructor to > make sure that m_display is set first. Otherwise we'd have to rely on the > order in which m_display and m_ownedDisplay are declared in the class > definition. Isn't the order ensured by the compiler? why do we have a warning when the order in the list doesn't match the one in the declaration, then? 1186935 5 277247 cgarcia 2016-04-25 08:10:11 -0700 Comment on attachment 277247 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=277247&action=review > Source/WebCore/platform/graphics/x11/PlatformDisplayX11.cpp:41 > + m_ownedDisplay = m_display != nullptr; We don't explicitly check nullptr, m_ownedDisplay = m_display; should just work, I think. 1186974 6 mcatanzaro 2016-04-25 10:26:08 -0700 (In reply to comment #4) > Isn't the order ensured by the compiler? why do we have a warning when the > order in the list doesn't match the one in the declaration, then? Member variables are always initialized in the order they are declared in the class, NOT the order they are listed in the initializer list. Putting them out of order in the initializer list is thus very confusing, and worth warning about. We should never do that in WebKit. Since relying on the order of declarations is risky -- someone could easily swap those in a "cleanup" and not notice the warning -- I agree with Berto that it's a good idea to do the assignment in the body of the constructor. 1187044 7 berto 2016-04-25 13:47:47 -0700 Committed r200046: <http://trac.webkit.org/changeset/200046> 277239 2016-04-25 04:53:58 -0700 2016-04-25 07:37:46 -0700 Patch 156972.diff text/plain 1295 berto ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCAxOTc2MGIyLi5jMDkyYmY4IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTYg QEAKKzIwMTYtMDQtMjUgIEFsYmVydG8gR2FyY2lhICA8YmVydG9AaWdhbGlhLmNvbT4KKworICAg ICAgICBbR1RLXSBDcmFzaGVzIGlmIERJU1BMQVkgaXMgdW5zZXQKKyAgICAgICAgaHR0cHM6Ly9i dWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE1Njk3MgorCisgICAgICAgIFJldmlld2Vk IGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIElmIERJU1BMQVkgaXMgdW5zZXQgdGhlbiBt X2Rpc3BsYXkgd2lsbCBiZSBOVUxMLCBjcmFzaGluZyBXZWJLaXQKKyAgICAgICAgd2hlbiBYQ2xv c2VEaXNwbGF5IGlzIGNhbGxlZCBpbiB0aGUgUGxhdGZvcm1EaXNwbGF5WDExIGRlc3RydWN0b3Iu CisKKyAgICAgICAgKiBwbGF0Zm9ybS9ncmFwaGljcy94MTEvUGxhdGZvcm1EaXNwbGF5WDExLmNw cDoKKyAgICAgICAgKFdlYkNvcmU6OlBsYXRmb3JtRGlzcGxheVgxMTo6flBsYXRmb3JtRGlzcGxh eVgxMSk6CisKIDIwMTYtMDQtMjUgIFlvdWVubiBGYWJsZXQgIDx5b3Vlbm4uZmFibGV0QGNyZi5j YW5vbi5mcj4KIAogICAgICAgICBEcm9wIFtVc2VQb2ludGVyc0V2ZW5Gb3JOb25OdWxsYWJsZU9i amVjdEFyZ3VtZW50c10gZnJvbSBUZXh0VHJhY2sKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3Jl L3BsYXRmb3JtL2dyYXBoaWNzL3gxMS9QbGF0Zm9ybURpc3BsYXlYMTEuY3BwIGIvU291cmNlL1dl YkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MveDExL1BsYXRmb3JtRGlzcGxheVgxMS5jcHAKaW5kZXgg OTdjODcyZC4uMWM0MGU3YzQgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dy YXBoaWNzL3gxMS9QbGF0Zm9ybURpc3BsYXlYMTEuY3BwCisrKyBiL1NvdXJjZS9XZWJDb3JlL3Bs YXRmb3JtL2dyYXBoaWNzL3gxMS9QbGF0Zm9ybURpc3BsYXlYMTEuY3BwCkBAIC00OSw3ICs0OSw3 IEBAIFBsYXRmb3JtRGlzcGxheVgxMTo6UGxhdGZvcm1EaXNwbGF5WDExKERpc3BsYXkqIGRpc3Bs YXkpCiAKIFBsYXRmb3JtRGlzcGxheVgxMTo6flBsYXRmb3JtRGlzcGxheVgxMSgpCiB7Ci0gICAg aWYgKG1fb3duZWREaXNwbGF5KQorICAgIGlmIChtX293bmVkRGlzcGxheSAmJiBtX2Rpc3BsYXkp CiAgICAgICAgIFhDbG9zZURpc3BsYXkobV9kaXNwbGF5KTsKIH0KIAo= 277247 2016-04-25 07:37:46 -0700 2016-04-25 08:10:11 -0700 Patch 156972.diff text/plain 1328 berto ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCAxOTc2MGIyLi5jMDkyYmY4IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTYg QEAKKzIwMTYtMDQtMjUgIEFsYmVydG8gR2FyY2lhICA8YmVydG9AaWdhbGlhLmNvbT4KKworICAg ICAgICBbR1RLXSBDcmFzaGVzIGlmIERJU1BMQVkgaXMgdW5zZXQKKyAgICAgICAgaHR0cHM6Ly9i dWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE1Njk3MgorCisgICAgICAgIFJldmlld2Vk IGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIElmIERJU1BMQVkgaXMgdW5zZXQgdGhlbiBt X2Rpc3BsYXkgd2lsbCBiZSBOVUxMLCBjcmFzaGluZyBXZWJLaXQKKyAgICAgICAgd2hlbiBYQ2xv c2VEaXNwbGF5IGlzIGNhbGxlZCBpbiB0aGUgUGxhdGZvcm1EaXNwbGF5WDExIGRlc3RydWN0b3Iu CisKKyAgICAgICAgKiBwbGF0Zm9ybS9ncmFwaGljcy94MTEvUGxhdGZvcm1EaXNwbGF5WDExLmNw cDoKKyAgICAgICAgKFdlYkNvcmU6OlBsYXRmb3JtRGlzcGxheVgxMTo6flBsYXRmb3JtRGlzcGxh eVgxMSk6CisKIDIwMTYtMDQtMjUgIFlvdWVubiBGYWJsZXQgIDx5b3Vlbm4uZmFibGV0QGNyZi5j YW5vbi5mcj4KIAogICAgICAgICBEcm9wIFtVc2VQb2ludGVyc0V2ZW5Gb3JOb25OdWxsYWJsZU9i amVjdEFyZ3VtZW50c10gZnJvbSBUZXh0VHJhY2sKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3Jl L3BsYXRmb3JtL2dyYXBoaWNzL3gxMS9QbGF0Zm9ybURpc3BsYXlYMTEuY3BwIGIvU291cmNlL1dl YkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MveDExL1BsYXRmb3JtRGlzcGxheVgxMS5jcHAKaW5kZXgg OTdjODcyZC4uOTY2YWNkNiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3Jh cGhpY3MveDExL1BsYXRmb3JtRGlzcGxheVgxMS5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUvcGxh dGZvcm0vZ3JhcGhpY3MveDExL1BsYXRmb3JtRGlzcGxheVgxMS5jcHAKQEAgLTM3LDggKzM3LDgg QEAgbmFtZXNwYWNlIFdlYkNvcmUgewogCiBQbGF0Zm9ybURpc3BsYXlYMTE6OlBsYXRmb3JtRGlz cGxheVgxMSgpCiAgICAgOiBtX2Rpc3BsYXkoWE9wZW5EaXNwbGF5KG51bGxwdHIpKQotICAgICwg bV9vd25lZERpc3BsYXkodHJ1ZSkKIHsKKyAgICBtX293bmVkRGlzcGxheSA9IG1fZGlzcGxheSAh PSBudWxscHRyOwogfQogCiBQbGF0Zm9ybURpc3BsYXlYMTE6OlBsYXRmb3JtRGlzcGxheVgxMShE aXNwbGF5KiBkaXNwbGF5KQo=