Fix public destructors on reference-counted classes

RESOLVED FIXED in Firefox 39

Status

Firefox Build System
General
RESOLVED FIXED
3 years ago
4 months ago

People

(Reporter: gerard, Assigned: tzimmermann)

Tracking

unspecified
mozilla39
x86_64
Linux
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(20 attachments, 2 obsolete attachments)

865 bytes, patch
sotaro
: review+
Details | Diff | Splinter Review
1.83 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
883 bytes, patch
sotaro
: review+
Details | Diff | Splinter Review
3.03 KB, patch
pzhang
: review+
Details | Diff | Splinter Review
7.35 KB, patch
sworkman
: review+
Details | Diff | Splinter Review
822 bytes, patch
mcmanus
: review+
Details | Diff | Splinter Review
4.94 KB, patch
dhylands
: review+
Details | Diff | Splinter Review
5.46 KB, patch
dhylands
: review+
Details | Diff | Splinter Review
1.43 KB, patch
dhylands
: review+
Details | Diff | Splinter Review
1.74 KB, patch
dhylands
: review+
Details | Diff | Splinter Review
3.05 KB, patch
dhylands
: review+
Details | Diff | Splinter Review
1.40 KB, patch
dhylands
: review+
Details | Diff | Splinter Review
545 bytes, patch
mwu
: review+
Details | Diff | Splinter Review
1.39 KB, patch
mwu
: review+
Details | Diff | Splinter Review
796 bytes, patch
mwu
: review+
Details | Diff | Splinter Review
812 bytes, patch
jrmuizel
: review+
Details | Diff | Splinter Review
22.46 KB, patch
shawnjohnjr
: review+
Details | Diff | Splinter Review
1.24 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
1.12 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
1.91 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
(Reporter)

Description

3 years ago
Let's track all of the instances of this issue. This seems not to be visible because it needs building with B2G_RIL with a compiler recent enough. That is happening in the context of bug 1038606
(Reporter)

Updated

3 years ago
Depends on: 1137155
Assignee: nobody → tzimmermann
Status: NEW → ASSIGNED
Trying...

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=81af1d16e95e

I build this patch set successfully on nexus-5-l, flatfish, unagi, and pandaboard.
Created attachment 8571302 [details] [diff] [review]
[01] Bug 1137151: Marked destructor of |android::MediaCodecReader| as protected
Attachment #8571302 - Flags: review?(sotaro.ikeda.g)
Created attachment 8571303 [details] [diff] [review]
[02] Bug 1137151: Remove ref-counting from |OMXVideoEncoder|
Attachment #8571303 - Flags: review?(sotaro.ikeda.g)
Created attachment 8571304 [details] [diff] [review]
[03] Bug 1137151: Marked destructor of |MuxerOperation| as protected
Attachment #8571304 - Flags: review?(sotaro.ikeda.g)
Created attachment 8571305 [details] [diff] [review]
[04] Bug 1137151: Marked destructor of |MozNDEFRecord| as protected
Attachment #8571305 - Flags: review?(allstars.chh)
Created attachment 8571307 [details] [diff] [review]
[05] Bug 1137151: Marked destructors of refcounted FM-radio classes as protected
Attachment #8571307 - Flags: review?(pzhang)
Created attachment 8571308 [details] [diff] [review]
[06] Bug 1137151: Marked destructors of ref-counted RTSP classes as protected
Attachment #8571308 - Flags: review?(mcmanus)
Created attachment 8571309 [details] [diff] [review]
[07] Bug 1137151: Mark destructor of |STSThreadPoolListener| as protected
Attachment #8571309 - Flags: review?(mcmanus)
Attachment #8571309 - Attachment description: Bug 1137151: Mark destructor of |STSThreadPoolListener| as protected → [07] Bug 1137151: Mark destructor of |STSThreadPoolListener| as protected
Created attachment 8571310 [details] [diff] [review]
[08] Bug 1137151: Marked destructors of ref-counted auto-mounter classes as protected
Attachment #8571310 - Flags: review?(dhylands)
Created attachment 8571311 [details] [diff] [review]
[09] Bug 1137151: Marked destructors of ref-counted MTP classes as protected
Attachment #8571311 - Flags: review?(dhylands)
Created attachment 8571313 [details] [diff] [review]
[10] Bug 1137151: Marked destructors of ref-counted time-zone classes as protected
Attachment #8571313 - Flags: review?(dhylands)
Created attachment 8571315 [details] [diff] [review]
[11] Bug 1137151: Marked destructors of ref-counted audio-manager classes as protected
Attachment #8571315 - Flags: review?(dhylands)
Created attachment 8571316 [details] [diff] [review]
[12] Bug 1137151: Marked destructors of ref-counted GonkHAL classes as protected
Attachment #8571316 - Flags: review?(dhylands)
Created attachment 8571317 [details] [diff] [review]
[13] Bug 1137151: Marked destructor of |MemoryPressureWatcher| as protected
Attachment #8571317 - Flags: review?(mwu)
Created attachment 8571318 [details] [diff] [review]
[14] Bug 1137151: Marked destructor of |GeckoTouchDispatcher| as protected
Attachment #8571318 - Flags: review?(mwu)
Created attachment 8571319 [details] [diff] [review]
[15] Bug 1137151: Marked destructor of |nsClipboard| as protected
Attachment #8571319 - Flags: review?(mwu)
Created attachment 8571321 [details] [diff] [review]
[16] Bug 1137151: Marked destructor of |nsWindow| as protected
Attachment #8571321 - Flags: review?(mwu)
Created attachment 8571322 [details] [diff] [review]
[17] Bug 1137151: Marked destructor of |nsScreenManagerGonk| as protected
Attachment #8571322 - Flags: review?(mwu)
Created attachment 8571323 [details] [diff] [review]
[18] Bug 1137151: Marked destructor of |GrallocReporter| as protected
Attachment #8571323 - Flags: review?(jmuizelaar)
Created attachment 8571324 [details] [diff] [review]
[19] Bug 1137151: Marked destructors of ref-counted Bluetooth classes as protected
Attachment #8571324 - Flags: review?(shuang)
Created attachment 8571413 [details] [diff] [review]
[20] Bug 1137151: Enable test for non-public ref-counted destructors on gcc 4.8 and later
Attachment #8571413 - Flags: review?(nfroyd)
Attachment #8571308 - Flags: review?(mcmanus) → review?(sworkman)
Attachment #8571309 - Flags: review?(mcmanus) → review+
Attachment #8571308 - Flags: review?(sworkman) → review+

Updated

3 years ago
Attachment #8571302 - Flags: review?(sotaro.ikeda.g) → review+

Updated

3 years ago
Attachment #8571303 - Flags: review?(sotaro.ikeda.g) → review+

Updated

3 years ago
Attachment #8571304 - Flags: review?(sotaro.ikeda.g) → review+
Comment on attachment 8571316 [details] [diff] [review]
[12] Bug 1137151: Marked destructors of ref-counted GonkHAL classes as protected

Review of attachment 8571316 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.
Attachment #8571316 - Flags: review?(dhylands) → review+
Attachment #8571310 - Flags: review?(dhylands) → review+
Attachment #8571311 - Flags: review?(dhylands) → review+
Attachment #8571313 - Flags: review?(dhylands) → review+
Attachment #8571315 - Flags: review?(dhylands) → review+
Attachment #8571305 - Flags: review?(allstars.chh) → review+
Attachment #8571307 - Flags: review?(pzhang) → review+
Comment on attachment 8571324 [details] [diff] [review]
[19] Bug 1137151: Marked destructors of ref-counted Bluetooth classes as protected

Review of attachment 8571324 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, thanks for fixing this bug.
Attachment #8571324 - Flags: review?(shuang) → review+

Comment 25

3 years ago
Comment on attachment 8571317 [details] [diff] [review]
[13] Bug 1137151: Marked destructor of |MemoryPressureWatcher| as protected

Review of attachment 8571317 [details] [diff] [review]:
-----------------------------------------------------------------

Redirecting review to someone who probably knows this code better than I do. (judging by the commit log)
Attachment #8571317 - Flags: review?(mwu) → review?(dhylands)

Updated

3 years ago
Attachment #8571318 - Flags: review?(mwu) → review+

Updated

3 years ago
Attachment #8571319 - Flags: review?(mwu) → review+

Updated

3 years ago
Attachment #8571321 - Flags: review?(mwu) → review+

Updated

3 years ago
Attachment #8571322 - Flags: review?(mwu) → review+
Comment on attachment 8571413 [details] [diff] [review]
[20] Bug 1137151: Enable test for non-public ref-counted destructors on gcc 4.8 and later

Review of attachment 8571413 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/glue/nsISupportsImpl.h
@@ +49,5 @@
>  #  if MOZ_USING_LIBSTDCXX && MOZ_GCC_VERSION_AT_LEAST(4, 8, 0)
>  #    define MOZ_HAVE_STD_IS_DESTRUCTIBLE
>     // Some GCC versions have an ICE when using destructors in decltype().
> +   // Works on GCC 4.8 at least.
> +#  elif MOZ_GCC_VERSION_AT_LEAST(4, 8, 0)

Out of curiosity, is that GCC 4.8.0 as provided by a distro (presumably with distro-specific patches applied) or GCC 4.8.0 straight from the release tarball?  And have you tested 4.8.1?  I couldn't really find commentary on why this is 4.8.2, but presumably somebody tested it with GCC 4.8.0 and/or 4.8.1 and found problems...
Attachment #8571413 - Flags: review?(nfroyd)
(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #26)
> 
> Out of curiosity, is that GCC 4.8.0 as provided by a distro (presumably with
> distro-specific patches applied) or GCC 4.8.0 straight from the release
> tarball?  And have you tested 4.8.1?  I couldn't really find commentary on
> why this is 4.8.2, but presumably somebody tested it with GCC 4.8.0 and/or
> 4.8.1 and found problems...

Hmm, the original comment sounded as if someone tried with gcc 4.8.2 and found it actually working; but gcc 4.8.1 or 4.8.0 was not available for testing.

The gcc 4.8.0, which I'm using comes prebuilt from the CAF repositories [1] we use for B2G. I haven't tried with gcc 4.8.1 or later. I don't know if there's a prebuilt binary somewhere and building one seems too much work for this simple bug.

[1] https://www.codeaurora.org/cgit/quic/la/platform/prebuilts/gcc/linux-x86/arm/arm-linux-androideabi-4.8/
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #27)
> (In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #26)
> > 
> > Out of curiosity, is that GCC 4.8.0 as provided by a distro (presumably with
> > distro-specific patches applied) or GCC 4.8.0 straight from the release
> > tarball?  And have you tested 4.8.1?  I couldn't really find commentary on
> > why this is 4.8.2, but presumably somebody tested it with GCC 4.8.0 and/or
> > 4.8.1 and found problems...
> 
> Hmm, the original comment sounded as if someone tried with gcc 4.8.2 and
> found it actually working; but gcc 4.8.1 or 4.8.0 was not available for
> testing.

Hm, I assumed that it didn't work on 4.8, given that the 4.8.2 bit was #else'd against a 4.8.0 conditional.  But you have a point, somebody might have just been conservative.

> The gcc 4.8.0, which I'm using comes prebuilt from the CAF repositories [1]
> we use for B2G. I haven't tried with gcc 4.8.1 or later. I don't know if
> there's a prebuilt binary somewhere and building one seems too much work for
> this simple bug.

Sure, toolchain building to resolve something like this would be silly.

Well, somebody will complain if this goes badly wrong, so let's try it.
Attachment #8571413 - Flags: review+
Attachment #8571317 - Flags: review?(dhylands) → review+
Summary: [meta] Fix public destructors on reference-counted classes → Fix public destructors on reference-counted classes
Created attachment 8575253 [details] [diff] [review]
[04] Bug 1137151: Marked destructor of |MozNDEFRecord| as protected (v2)

Changes since v1:

  - rebased onto latest m-c
Attachment #8571305 - Attachment is obsolete: true
Attachment #8575253 - Flags: review+
Created attachment 8575254 [details] [diff] [review]
[14] Bug 1137151: Marked destructor of |GeckoTouchDispatcher| as protected (v2)

Changes since v1:

  - rebased onto latest m-c
Attachment #8571318 - Attachment is obsolete: true
Attachment #8575254 - Flags: review+
This seemed to have broken bluetooth functionality : bug 1142132.
Backed out the bluetooth related cset in https://hg.mozilla.org/mozilla-central/rev/c33922ee3ac3 due to a smoketest bustage : https://bugzilla.mozilla.org/show_bug.cgi?id=1142132#c9
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Relanded that piece in https://hg.mozilla.org/mozilla-central/rev/e1262bdb5b14 because it broke b2g emulator builds: 

https://treeherder.mozilla.org/logviewer.html#?job_id=1150744&repo=mozilla-central

I guess this re-resolves this bug, though something will need to be done to fix bug 1142132.
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Flags: needinfo?(tzimmermann)
Resolution: --- → FIXED
If you're backing out any piece of this (which means you're adding back some public destructors), I suspect you *also* need to backout part 20 (which is the part that makes public destructors fatal, on the GCC version that we use on our builders).
So, it's expected that comment 34 would've broken builds -- it needed to also backout part 20 (mozilla-central changeset d5e5fcb29452) in order to still build successfully.

(Having said that -- I don't immediately see how the bluetooth-related cset here (c33922ee3ac3) could actually affect behavior. I did find one refcounting bug in related code, from code-inspection of BluetoothHALInterfaceRunnable0-related-code (the code that's crashing in the regression here), and I filed bug 1142364 on it. It's remotely-possible that that bug's really what's responsible for the crash, and this just exposed it somehow.)
I take a look at bug 1142132.
Flags: needinfo?(tzimmermann)

Updated

4 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.