Closed Bug 1137151 Opened 9 years ago Closed 9 years ago

Fix public destructors on reference-counted classes

Categories

(Firefox Build System :: General, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(firefox39 fixed)

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: gerard-majax, Assigned: tzimmermann)

References

Details

Attachments

(20 files, 2 obsolete files)

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
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
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.
Attachment #8571309 - Attachment description: Bug 1137151: Mark destructor of |STSThreadPoolListener| as protected → [07] Bug 1137151: Mark destructor of |STSThreadPoolListener| as protected
Attachment #8571323 - Flags: review?(jmuizelaar) → review+
Attachment #8571308 - Flags: review?(mcmanus) → review?(sworkman)
Attachment #8571309 - Flags: review?(mcmanus) → review+
Attachment #8571308 - Flags: review?(sworkman) → review+
Attachment #8571302 - Flags: review?(sotaro.ikeda.g) → review+
Attachment #8571303 - Flags: review?(sotaro.ikeda.g) → review+
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 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)
Attachment #8571318 - Flags: review?(mwu) → review+
Attachment #8571319 - Flags: review?(mwu) → review+
Attachment #8571321 - Flags: review?(mwu) → review+
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
Changes since v1:

  - rebased onto latest m-c
Attachment #8571305 - Attachment is obsolete: true
Attachment #8575253 - Flags: review+
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
Closed: 9 years ago9 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)
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: