Bug 34434 – [Android] AbstractWeakReferenceMap in V8DOMMap.h needs a virtual destructor

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Steve Block]

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

[Adam Barth]

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.)

[Steve Block]

(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.

[Steve Block]

Created attachment 47853 [details]
Patch

[Steve Block]

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.

[Adam Barth]

+ 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.

[Adam Barth]

> 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.

[Steve Block]

> + 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.

[Steve Block]

> 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.

[Steve Block]

Comment on attachment 47853 [details]
Patch

Email from abarth:
> Ah, that's right.  r=me.

[WebKit Commit Bot]

Comment on attachment 47853 [details]
Patch

Rejecting patch 47853 from review queue.

[e-mail] 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 [e-mail] to request a commit-queue restart.  After restart the commit-queue will correctly respect your reviewer rights.

[Steve Block]

Comment on attachment 47853 [details]
Patch

Will land manually

[Steve Block]

Landed manually as http://trac.webkit.org/changeset/54218

Closing bug as resolved fixed.

[Adam Barth]

Sorry for not flipping the bit on the patch myself.  I was away from my computer yesterday.

[Eric Seidel (no email)]

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.