1025576 - Double delete leads to crash @TSymbolTableLevel::~TSymbolTableLevel in gcc 4.9 builds when using WebGL

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Mike Hommey [:glandium]]

GCC 4.9 doesn't put pointers to virtual destructors in pure virtual classes anymore.

In practice, this means the TSymbol vtable looks like this:
0
0
_ZNK7TSymbol14getMangledNameEv
_ZNK7TSymbol10isFunctionEv
_ZNK7TSymbol10isVariableEv
__cxa_pure_virtual

instead of:
_ZN7TSymbolD2Ev
_ZN7TSymbolD0Ev
_ZNK7TSymbol14getMangledNameEv
_ZNK7TSymbol10isFunctionEv
_ZNK7TSymbol10isVariableEv
__cxa_pure_virtual

(which is what we currently have with our builds with gcc 4.7)

For some reason, some TSymbols are added twice to TSymbolTableLevel, so when its destructor is called, it enumerates all TSymbols, and deletes them one by one.
TSymbol comes with its own delete that doesn't call free(), so TSymbols are not deallocated, but still, their destructor run. So for example, when a TFunction is deleted, the TFunction destructor is called, then the vtable pointer is changed to point to TSymbol and the TSymbol destructor is called for the same object.
The second time the same object is deleted, its vtable pointer still points to TSymbol instead of whatever it was before (TFunction in the example above), so calling the virtual destructor crashes, since it's a NULL pointer. We're lucky that the TSymbol destructor does nothing, because this could have nasty effects even with gcc 4.7 builds.

I haven't dug deep enough to know why we're inserting the same object twice.

Comment 1

[Mike Hommey [:glandium]]

So, the one I'm looking is inserted once from:
  http://hg.mozilla.org/mozilla-central/file/a6a457fe2a2a/gfx/angle/src/compiler/glslang_tab.cpp#l3230
and another time from:
  http://hg.mozilla.org/mozilla-central/file/a6a457fe2a2a/gfx/angle/src/compiler/glslang_tab.cpp#l3242

The second time ends up in
  http://hg.mozilla.org/mozilla-central/file/a6a457fe2a2a/gfx/angle/src/compiler/SymbolTable.h#l205

So, we're essentially inserting the same TSymbol against its name and against its mangled name. I have no idea whether this is necessary to the code using that table, but it certainly doesn't make the destructor do the right thing.

Comment 2

[Mike Hommey [:glandium]]

Heh, the comment just above the first one explicitly says this is done on purpose. So it's the destructor that needs fixing.

Comment 3

[Mike Hommey [:glandium]]

Attachment: Fix crash in TSymbolTableLevel::~TSymbolTableLevel with GCC 4.9

Comment 4

[Benoit Jacob [:bjacob] (mostly away)]

Jeff: is this something that you already know about and/or have comments about? Happy to review otherwise.

Comment 5

[Kelsey Gilbert [:jgilbert]]

(In reply to Benoit Jacob [:bjacob] from comment #4)
> Jeff: is this something that you already know about and/or have comments
> about? Happy to review otherwise.

Nope, go ahead.

Comment 6

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 8440362 [details] [diff] [review]
Fix crash in TSymbolTableLevel::~TSymbolTableLevel with GCC 4.9

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

r+ as that does come with a clear explanation of how this avoids a nasty crash. But this is scary stuff. The upstream bug should really get more attention than it's received so far.

Comment 7

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d61342b2da5e

Comment 8

[Mike Hommey [:glandium]]

Comment on attachment 8440362 [details] [diff] [review]
Fix crash in TSymbolTableLevel::~TSymbolTableLevel with GCC 4.9

[Approval Request Comment]
Regression caused by (bug #): regression from bug 883478
User impact if declined: WebGL crashes when Firefox is built with GCC 4.9, which Linux distros are doing. For this reason, if we ever end up doing a chemspill for 30, we should take this in at the same time.
Testing completed (on m-c, etc.): Just landed on m-i, but tested locally.
Risk to taking this patch (and alternatives if risky): Low. The only risk from this patch is that some objects could be left not deleted, but there's only one place that inserts entries with a name that is not mangled, and the same place inserts the same entry with the mangled name as well (which is what the bug is all about)
String or IDL/UUID changes made by this patch: None

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d61342b2da5e

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/83f7cdb68bf2
https://hg.mozilla.org/releases/mozilla-beta/rev/0132fb8d61f7

Comment 11

[Lukas Blakk [:lsblakk] use ?needinfo]

So far there is no driver for a 30.0.1, leaving this nomination alive so it's on our radar should a need arise.

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8440362 [details] [diff] [review]
Fix crash in TSymbolTableLevel::~TSymbolTableLevel with GCC 4.9

31 is going to ship next week. So, not taking the patch in release.