34434 2010-02-01 09:38:18 -0800 [Android] AbstractWeakReferenceMap in V8DOMMap.h needs a virtual destructor 2010-02-02 13:54:00 -0800 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) Android Android RESOLVED FIXED P2 Normal --- 1 steveblock webkit-unassigned abarth android-webkit-unforking eric steveblock oldest_to_newest 186456 0 steveblock 2010-02-01 09:38:18 -0800 AbstractWeakReferenceMap in V8DOMMap.h has virtual methods but no virtual destructor. This causes build errors on Android. This was added in http://trac.webkit.org/changeset/53944 186458 1 abarth 2010-02-01 09:48:33 -0800 Sorry about that. We need some automated way of catching these cases so we don't keep breaking the Android build for no reason. The ideal thing would be to set up an EWS bot to catch these errors. If you're interested in setting one up on your end, send me email and I'll tell you what's involved. (I would set one up in our current bot farm, but I don't know how to build Android.) 186463 2 steveblock 2010-02-01 09:56:21 -0800 (In reply to comment #1) > Sorry about that. No problem > We need some automated way of catching these cases so we > don't keep breaking the Android build for no reason. The ideal thing would be > to set up an EWS bot to catch these errors. If you're interested in setting > one up on your end, send me email and I'll tell you what's involved. (I would > set one up in our current bot farm, but I don't know how to build Android.) Agreed, thanks for the offer of help. Android isn't yet fully upstreamed, so can't be built from webkit.org. We're working as fast as we can to fix that and will set up a bot as soon as possible. 186476 3 47853 steveblock 2010-02-01 10:32:36 -0800 Created attachment 47853 Patch 186481 4 steveblock 2010-02-01 10:35:21 -0800 Patch adds virtual destructors for AbstractWeakReferenceMap and AbstractWeakReferenceMap::Visitor Note that the destructor for AbstractWeakReferenceMap needs to be public as it's used by ScopedDOMDataStore, but the destructor for AbstractWeakReferenceMap::Visitor does not. 186482 5 abarth 2010-02-01 10:38:43 -0800 + virtual ~AbstractWeakReferenceMap That's fine, but + virtual ~Visitor that's not correct. Objects are never deleted via Vistor pointers and inheriting from the Visitor interface shouldn't force an object to have a virtual destructor. I feel like we had that discussion on another bug and found another solution. 186483 6 abarth 2010-02-01 10:42:38 -0800 > Agreed, thanks for the offer of help. Android isn't yet fully upstreamed, so > can't be built from webkit.org. We're working as fast as we can to fix that and > will set up a bot as soon as possible. You can actually set up an EWS bot without the ability to build from webkit.org. You just need to fill in the update_webkit_command and the build_webkit_command methods in a AndroidPort class in this file: http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/webkitport.py You can make them call arbitrary scripts, so as long as you have a machine that can update and build Android's WebKit port from the command line, you can set up an EWS bot. 186486 7 steveblock 2010-02-01 10:50:28 -0800 > + virtual ~Visitor > > that's not correct. Objects are never deleted via Vistor pointers and > inheriting from the Visitor interface shouldn't force an object to have a > virtual destructor. I feel like we had that discussion on another bug and > found another solution. Are you thinking of Bug 33675 and http://trac.webkit.org/changeset/53401 ? This added a virtual destructor to WeakReferenceMap::Visitor. It was the last change to this file before http://trac.webkit.org/changeset/53944 removed WeakReferenceMap::Visitor and added AbstractWeakReferenceMap::Visitor. The outcome of the discussion was to make the destructor protected. 186487 8 steveblock 2010-02-01 10:57:12 -0800 > You can actually set up an EWS bot without the ability to build from > webkit.org. You just need to fill in the update_webkit_command and the > build_webkit_command methods in a AndroidPort class in this file: > > http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/webkitport.py > > You can make them call arbitrary scripts, so as long as you have a machine that > can update and build Android's WebKit port from the command line, you can set > up an EWS bot. Ah, good to know, thanks. The problem still remains that because Android WebKit is forked from from webkit.org, there's a good chance that a patch won't apply, so we can't update the tree without manual conflict resolution. There would be lots of false positives, but it might be worth having anyway as it will give us a heads up as soon as a conflicting patch is landed. I'll look into it. 186859 9 47853 steveblock 2010-02-02 01:30:57 -0800 Comment on attachment 47853 Patch Email from abarth: > Ah, that's right. r=me. 186865 10 47853 commit-queue 2010-02-02 01:46:39 -0800 Comment on attachment 47853 Patch Rejecting patch 47853 from review queue. steveblock@google.com does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/committers.py. - If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have reviewer rights please correct the error in WebKitTools/Scripts/webkitpy/committers.py by adding yourself to the file (no review needed). Due to bug 30084 the commit-queue will require a restart after your change. Please contact eseidel@chromium.org to request a commit-queue restart. After restart the commit-queue will correctly respect your reviewer rights. 186866 11 47853 steveblock 2010-02-02 01:52:24 -0800 Comment on attachment 47853 Patch Will land manually 186869 12 steveblock 2010-02-02 01:59:47 -0800 Landed manually as http://trac.webkit.org/changeset/54218 Closing bug as resolved fixed. 186959 13 abarth 2010-02-02 07:39:18 -0800 Sorry for not flipping the bit on the patch myself. I was away from my computer yesterday. 187121 14 eric 2010-02-02 13:54:00 -0800 You can't r+ patches in bugzilla if you're not a reviewer. However you can still use the commit-queue. cq+'d patches will the review flag unset, will still be landed. But they'll fail to land if they have an OOPS in them, so you'd have to set the reviewer before hand or replace the reviewer line with some text about why review was not required. Adam Barth was working on some sort of "webkit-patch land-safely" command which used this feature of the commit-queue. 47853 2010-02-01 10:32:36 -0800 2010-02-02 01:52:23 -0800 Patch bug-34434-20100201183234.patch text/plain 1532 steveblock SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA1NDEzNykKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTYgQEAKKzIwMTAtMDItMDEgIFN0ZXZlIEJsb2NrICA8c3RldmVibG9ja0Bnb29n bGUuY29tPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAg IEFkZHMgdmlydHVhbCBkZXN0cnVjdG9ycyBmb3IgQWJzdHJhY3RXZWFrUmVmZXJlbmNlTWFwIGFu ZCBBYnN0cmFjdFdlYWtSZWZlcmVuY2VNYXA6OlZpc2l0b3IKKyAgICAgICAgaHR0cHM6Ly9idWdz LndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTM0NDM0CisKKyAgICAgICAgTm8gbmV3IHRlc3Rz LCBidWlsZCBmaXggb25seS4KKworICAgICAgICAqIGJpbmRpbmdzL3Y4L1Y4RE9NTWFwLmg6IE1v ZGlmaWVkLgorICAgICAgICAoV2ViQ29yZTo6QWJzdHJhY3RXZWFrUmVmZXJlbmNlTWFwOjp+QWJz dHJhY3RXZWFrUmVmZXJlbmNlTWFwKTogQWRkZWQuCisgICAgICAgIChXZWJDb3JlOjpBYnN0cmFj dFdlYWtSZWZlcmVuY2VNYXA6OlZpc2l0b3I6On5WaXNpdG9yKTogQWRkZWQuCisKIDIwMTAtMDIt MDEgIERpcmsgU2NodWx6ZSAgPGtyaXRAd2Via2l0Lm9yZz4KIAogICAgICAgICBSZXZpZXdlZCBi eSBTaW1vbiBGcmFzZXIuCkluZGV4OiBXZWJDb3JlL2JpbmRpbmdzL3Y4L1Y4RE9NTWFwLmgKPT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PQotLS0gV2ViQ29yZS9iaW5kaW5ncy92OC9WOERPTU1hcC5oCShyZXZpc2lvbiA1NDEz NikKKysrIFdlYkNvcmUvYmluZGluZ3MvdjgvVjhET01NYXAuaAkod29ya2luZyBjb3B5KQpAQCAt NDQsMTAgKzQ0LDEzIEBAIG5hbWVzcGFjZSBXZWJDb3JlIHsKICAgICB0ZW1wbGF0ZSA8Y2xhc3Mg S2V5VHlwZSwgY2xhc3MgVmFsdWVUeXBlPiBjbGFzcyBBYnN0cmFjdFdlYWtSZWZlcmVuY2VNYXAg ewogICAgIHB1YmxpYzoKICAgICAgICAgQWJzdHJhY3RXZWFrUmVmZXJlbmNlTWFwKHY4OjpXZWFr UmVmZXJlbmNlQ2FsbGJhY2sgY2FsbGJhY2spIDogbV93ZWFrUmVmZXJlbmNlQ2FsbGJhY2soY2Fs bGJhY2spIHsgfQorICAgICAgICB2aXJ0dWFsIH5BYnN0cmFjdFdlYWtSZWZlcmVuY2VNYXAoKSB7 IH0KIAogICAgICAgICBjbGFzcyBWaXNpdG9yIHsKICAgICAgICAgcHVibGljOgogICAgICAgICAg ICAgdmlydHVhbCB2b2lkIHZpc2l0RE9NV3JhcHBlcihLZXlUeXBlKiBrZXksIHY4OjpQZXJzaXN0 ZW50PFZhbHVlVHlwZT4gb2JqZWN0KSA9IDA7CisgICAgICAgIHByb3RlY3RlZDoKKyAgICAgICAg ICAgIHZpcnR1YWwgflZpc2l0b3IoKSB7IH0KICAgICAgICAgfTsKIAogICAgICAgICB2aXJ0dWFs IHY4OjpQZXJzaXN0ZW50PFZhbHVlVHlwZT4gZ2V0KEtleVR5cGUqIG9iaikgPSAwOwo=