1353941 - clang 3.8 crashes while parsing an assertion in nsExpirationTracker.h

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Daniel Holbert [:dholbert]]

It seems some new code from bug 1345464 is lucky enough to trigger a bug in clang 3.8 (which means build failure).

I'll attach a text file with all the output, but the gist of it is:
===========
1.	../../../dist/include/nsExpirationTracker.h:120:7 <Spelling=../../../dist/include/mozilla/Assertions.h:402:71>: current parser token '>'
2.	../../../dist/include/nsExpirationTracker.h:92:1: parsing struct/union/class body 'ExpirationTrackerImpl'
3.	../../../dist/include/nsExpirationTracker.h:114:3: parsing function body 'ExpirationTrackerImpl<T, K, Mutex, AutoLock>'
4.	../../../dist/include/nsExpirationTracker.h:114:3: in compound statement ('{}')
5.	../../../dist/include/nsExpirationTracker.h:118:23: in compound statement ('{}')
6.	../../../dist/include/nsExpirationTracker.h:120:7 <Spelling=../../../dist/include/mozilla/Assertions.h:426:6>: in compound statement ('{}')
clang: error: unable to execute command: Segmentation fault (core dumped)
clang: error: clang frontend command failed due to signal (use -v to see invocation)
clang version 3.8.1-12ubuntu1 (tags/RELEASE_381/final)
===========

This points to nsExpirationTracker.h:120 which is here:
https://dxr.mozilla.org/mozilla-central/rev/720b9177c6856c1c4339d0fac1bf5149c0d53950/xpcom/ds/nsExpirationTracker.h#120
...which is a MOZ_RELEASE_ASSERT that was just added over in bug 1345464.

I think this is the same clang bug that we've been working around in CSS Grid code with the CLANG_CRASH_BUG special-case.  Seems related at least -- an assertion inside of some templated code triggers a clang 3.8 crash.

We probably need to come up with a way to prevent this from biting Firefox developres, since my impression is that clang 3.8 is still in pretty wide use. (It's the default version of clang on Ubuntu 16.10, the current latest Ubuntu version. Not sure if it's also the default on 16.04 LTS.)

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: full clang error output

Here's the full error output, for reference.

(I believe this is fixed in newer versions of clang -- at least, if it's the same problem we hit in grid code, then it's definitely fixed for clang 3.9 and above, IIRC.)

Comment 2

[Daniel Holbert [:dholbert]]

Possible hackaround: looks like I can get past this if I convert the MOZ_RELEASE_ASSERT to an if-check + MOZ_CRASH...

Comment 3

[Eric Rahm [:erahm]]

(In reply to Daniel Holbert [:dholbert] from comment #0)
> We probably need to come up with a way to prevent this from biting Firefox
> developres, since my impression is that clang 3.8 is still in pretty wide
> use. (It's the default version of clang on Ubuntu 16.10, the current latest
> Ubuntu version. Not sure if it's also the default on 16.04 LTS.)

On 16.04.2 LTS it's: clang version 3.8.0-2ubuntu4

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: Bug 1353941: Convert a MOZ_RELEASE_ASSERT() expression to "if" + MOZ_CRASH(), to work around clang 3.8 segfault.

Review commit: https://reviewboard.mozilla.org/r/127004/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/127004/

Comment 5

[Daniel Holbert [:dholbert]]

Thanks Eric! I think we'll need to accommodate developers with clang 3.8 for the foreseeable future, then.  So a workaround is merited. --> Strawman patch attached. 

Notes:
 - My intent is to just directly invert the existing logic in my "if" check. The idea (in both the old & new code) is: if we can't authoritatively prove that the event target is on the current thread, then BAIL! 
 - I'm not doing the "CLANG_CRASH_BUG" thing that we do in CSS grid code, because we can't just skip this particular check -- it seems we depend on it for safety/sanity even in opt builds.

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 8855123 [details]
Bug 1353941: Convert a MOZ_RELEASE_ASSERT() expression to "if" + MOZ_CRASH(), to work around clang 3.8 segfault.

https://reviewboard.mozilla.org/r/127004/#review129742

Do you think it's worth a brief comment explaining why we're not using `MOZ_RELEASE_ASSERT` here?

Comment 7

[Daniel Holbert [:dholbert]]

Sure, I'll add one before landing.

Thanks for the review!

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 8855123 [details]
Bug 1353941: Convert a MOZ_RELEASE_ASSERT() expression to "if" + MOZ_CRASH(), to work around clang 3.8 segfault.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127004/diff/1-2/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5eecc08a74e3
Convert a MOZ_RELEASE_ASSERT() expression to "if" + MOZ_CRASH(), to work around clang 3.8 segfault. r=froydnj

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5eecc08a74e3