Closed
Bug 1137151
Opened 9 years ago
Closed 9 years ago
Fix public destructors on reference-counted classes
Categories
(Firefox Build System :: General, defect)
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
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → tzimmermann
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•9 years ago
|
||
Trying... https://treeherder.mozilla.org/#/jobs?repo=try&revision=81af1d16e95e I build this patch set successfully on nexus-5-l, flatfish, unagi, and pandaboard.
Assignee | ||
Comment 2•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=777aaec90637
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #8571302 -
Flags: review?(sotaro.ikeda.g)
Assignee | ||
Comment 4•9 years ago
|
||
Attachment #8571303 -
Flags: review?(sotaro.ikeda.g)
Assignee | ||
Comment 5•9 years ago
|
||
Attachment #8571304 -
Flags: review?(sotaro.ikeda.g)
Assignee | ||
Comment 6•9 years ago
|
||
Attachment #8571305 -
Flags: review?(allstars.chh)
Assignee | ||
Comment 7•9 years ago
|
||
Attachment #8571307 -
Flags: review?(pzhang)
Assignee | ||
Comment 8•9 years ago
|
||
Attachment #8571308 -
Flags: review?(mcmanus)
Assignee | ||
Comment 9•9 years ago
|
||
Attachment #8571309 -
Flags: review?(mcmanus)
Assignee | ||
Updated•9 years ago
|
Attachment #8571309 -
Attachment description: Bug 1137151: Mark destructor of |STSThreadPoolListener| as protected → [07] Bug 1137151: Mark destructor of |STSThreadPoolListener| as protected
Assignee | ||
Comment 10•9 years ago
|
||
Attachment #8571310 -
Flags: review?(dhylands)
Assignee | ||
Comment 11•9 years ago
|
||
Attachment #8571311 -
Flags: review?(dhylands)
Assignee | ||
Comment 12•9 years ago
|
||
Attachment #8571313 -
Flags: review?(dhylands)
Assignee | ||
Comment 13•9 years ago
|
||
Attachment #8571315 -
Flags: review?(dhylands)
Assignee | ||
Comment 14•9 years ago
|
||
Attachment #8571316 -
Flags: review?(dhylands)
Assignee | ||
Comment 15•9 years ago
|
||
Attachment #8571317 -
Flags: review?(mwu)
Assignee | ||
Comment 16•9 years ago
|
||
Attachment #8571318 -
Flags: review?(mwu)
Assignee | ||
Comment 17•9 years ago
|
||
Attachment #8571319 -
Flags: review?(mwu)
Assignee | ||
Comment 18•9 years ago
|
||
Attachment #8571321 -
Flags: review?(mwu)
Assignee | ||
Comment 19•9 years ago
|
||
Attachment #8571322 -
Flags: review?(mwu)
Assignee | ||
Comment 20•9 years ago
|
||
Attachment #8571323 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 21•9 years ago
|
||
Attachment #8571324 -
Flags: review?(shuang)
Updated•9 years ago
|
Attachment #8571323 -
Flags: review?(jmuizelaar) → review+
Assignee | ||
Comment 22•9 years ago
|
||
Attachment #8571413 -
Flags: review?(nfroyd)
Updated•9 years ago
|
Attachment #8571308 -
Flags: review?(mcmanus) → review?(sworkman)
Updated•9 years ago
|
Attachment #8571309 -
Flags: review?(mcmanus) → review+
Updated•9 years ago
|
Attachment #8571308 -
Flags: review?(sworkman) → review+
Updated•9 years ago
|
Attachment #8571302 -
Flags: review?(sotaro.ikeda.g) → review+
Updated•9 years ago
|
Attachment #8571303 -
Flags: review?(sotaro.ikeda.g) → review+
Updated•9 years ago
|
Attachment #8571304 -
Flags: review?(sotaro.ikeda.g) → review+
Comment 23•9 years ago
|
||
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+
Updated•9 years ago
|
Attachment #8571310 -
Flags: review?(dhylands) → review+
Updated•9 years ago
|
Attachment #8571311 -
Flags: review?(dhylands) → review+
Updated•9 years ago
|
Attachment #8571313 -
Flags: review?(dhylands) → review+
Updated•9 years ago
|
Attachment #8571315 -
Flags: review?(dhylands) → review+
Attachment #8571305 -
Flags: review?(allstars.chh) → review+
Updated•9 years ago
|
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•9 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•9 years ago
|
Attachment #8571318 -
Flags: review?(mwu) → review+
Updated•9 years ago
|
Attachment #8571319 -
Flags: review?(mwu) → review+
Updated•9 years ago
|
Attachment #8571321 -
Flags: review?(mwu) → review+
Updated•9 years ago
|
Attachment #8571322 -
Flags: review?(mwu) → review+
Comment 26•9 years ago
|
||
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)
Assignee | ||
Comment 27•9 years ago
|
||
(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/
Comment 28•9 years ago
|
||
(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.
Updated•9 years ago
|
Attachment #8571413 -
Flags: review+
Updated•9 years ago
|
Attachment #8571317 -
Flags: review?(dhylands) → review+
Assignee | ||
Updated•9 years ago
|
Summary: [meta] Fix public destructors on reference-counted classes → Fix public destructors on reference-counted classes
Assignee | ||
Comment 29•9 years ago
|
||
Changes since v1: - rebased onto latest m-c
Attachment #8571305 -
Attachment is obsolete: true
Attachment #8575253 -
Flags: review+
Assignee | ||
Comment 30•9 years ago
|
||
Changes since v1: - rebased onto latest m-c
Attachment #8571318 -
Attachment is obsolete: true
Attachment #8575254 -
Flags: review+
Assignee | ||
Comment 31•9 years ago
|
||
Thanks everyone! https://hg.mozilla.org/integration/b2g-inbound/rev/3d493b734419 https://hg.mozilla.org/integration/b2g-inbound/rev/4e473e4665b3 https://hg.mozilla.org/integration/b2g-inbound/rev/f9692c6fa585 https://hg.mozilla.org/integration/b2g-inbound/rev/5f11e6148aad https://hg.mozilla.org/integration/b2g-inbound/rev/5bec2dc6ceac https://hg.mozilla.org/integration/b2g-inbound/rev/e0687d17e8f8 https://hg.mozilla.org/integration/b2g-inbound/rev/6650bbc62372 https://hg.mozilla.org/integration/b2g-inbound/rev/191131b6b165 https://hg.mozilla.org/integration/b2g-inbound/rev/07597ca96575 https://hg.mozilla.org/integration/b2g-inbound/rev/9d6eb9ee78c3 https://hg.mozilla.org/integration/b2g-inbound/rev/34a622f3e1db https://hg.mozilla.org/integration/b2g-inbound/rev/c95d2c40c94c https://hg.mozilla.org/integration/b2g-inbound/rev/854abb194184 https://hg.mozilla.org/integration/b2g-inbound/rev/12d1a1d61c52 https://hg.mozilla.org/integration/b2g-inbound/rev/b61c178f6c55 https://hg.mozilla.org/integration/b2g-inbound/rev/ee68d8a9edcb https://hg.mozilla.org/integration/b2g-inbound/rev/10b2ee690fab https://hg.mozilla.org/integration/b2g-inbound/rev/da8b000c08c6 https://hg.mozilla.org/integration/b2g-inbound/rev/d645da56de29 https://hg.mozilla.org/integration/b2g-inbound/rev/d5e5fcb29452 https://treeherder.mozilla.org/#/jobs?repo=b2g-inbound&revision=d5e5fcb29452
https://hg.mozilla.org/mozilla-central/rev/3d493b734419 https://hg.mozilla.org/mozilla-central/rev/4e473e4665b3 https://hg.mozilla.org/mozilla-central/rev/f9692c6fa585 https://hg.mozilla.org/mozilla-central/rev/5f11e6148aad https://hg.mozilla.org/mozilla-central/rev/5bec2dc6ceac https://hg.mozilla.org/mozilla-central/rev/e0687d17e8f8 https://hg.mozilla.org/mozilla-central/rev/6650bbc62372 https://hg.mozilla.org/mozilla-central/rev/191131b6b165 https://hg.mozilla.org/mozilla-central/rev/07597ca96575 https://hg.mozilla.org/mozilla-central/rev/9d6eb9ee78c3 https://hg.mozilla.org/mozilla-central/rev/34a622f3e1db https://hg.mozilla.org/mozilla-central/rev/c95d2c40c94c https://hg.mozilla.org/mozilla-central/rev/854abb194184 https://hg.mozilla.org/mozilla-central/rev/12d1a1d61c52 https://hg.mozilla.org/mozilla-central/rev/b61c178f6c55 https://hg.mozilla.org/mozilla-central/rev/ee68d8a9edcb https://hg.mozilla.org/mozilla-central/rev/10b2ee690fab https://hg.mozilla.org/mozilla-central/rev/da8b000c08c6 https://hg.mozilla.org/mozilla-central/rev/d645da56de29 https://hg.mozilla.org/mozilla-central/rev/d5e5fcb29452
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
This seemed to have broken bluetooth functionality : bug 1142132.
Comment 34•9 years ago
|
||
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 ago → 9 years ago
Flags: needinfo?(tzimmermann)
Resolution: --- → FIXED
Comment 36•9 years ago
|
||
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).
Comment 37•9 years ago
|
||
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.)
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•