529540 - Unbounded recursion in Traits::resolveSignatures, countInterfaces, addInterfaces

Status: VERIFIED FIXED

(Tamarin Graveyard :: Virtual Machine, defect, P3)

Description

[Edwin Smith]

more unwanted recursion:

 * Traits::resolveSignatures recurses up the inheritance and the interface dag
 * Traits::countInterfaces and addInterfaces recurse up the interface dag, too.

Simple but somewhat heavy fix for each case is a working queue of Traits* to
visit.

Comment 1

[Edwin Smith]

Attachment: Fix unbounded recursion in Traits::countNewInterfaces()

Comment 2

[Lars T Hansen]

Comment on attachment 413605 [details] [diff] [review]
Fix unbounded recursion in Traits::countNewInterfaces()

Is every item we push onto the stack guarded against being garbage collected by virtue of being linked from some other structure that, unlike Stack<>, is visible to the GC?

Comment 3

[Edwin Smith]

Each interface is rooted by being present in PoolObject and Domain, obtained by calling pool->resolveTypeName.  I can add a comment to that effect or use List<GCobjects>; at this point making GC references obvious maybe outweighs any tiny performance gain.

Comment 4

[Lars T Hansen]

Comment on attachment 413605 [details] [diff] [review]
Fix unbounded recursion in Traits::countNewInterfaces()

A comment probably suffices.

(If we ever want a partly-moving GC we'll have to fix all cases like this, though - it becomes vital that references from unmanaged memory are all seen.)

Comment 5

[Edwin Smith]

Comment on attachment 413605 [details] [diff] [review]
Fix unbounded recursion in Traits::countNewInterfaces()

pushed http://hg.mozilla.org/tamarin-redux/rev/6297a734323d

Comment 6

[Edwin Smith]

Attachment: Fix unbounded recursion in Traits::resolveSignatures()

Comment 7

[Edwin Smith]

(In reply to comment #6)
> Created an attachment (id=414075) [details]
> Fix unbounded recursion in Traits::resolveSignatures()

The existing code will recurse into resolveSupertypes at various points in the middle; base types are handled first, interfaces of concrete types are handled while fixing up interface bindings, and finally interfaces of other interfaces are handled last.

This patch resolves all supertypes before doing anything on "self"; this is not beleived to cause problems and since even the old code ultimately would resolve all supertypes, it is beleived that this won't make anything get resolved materially sooner than before.

Since we have arrays holding all supertypes (m_primary_supertypes and m_secondary_supertypes), the fix is to just put those types in top-down order, without recursion, then iterate through and resolve each type.

Comment 8

[Edwin Smith]

Attachment: no recursion in Traits::resolveSignatures() (2nd try)

Comment 9

[Lars T Hansen]

Comment on attachment 414078 [details] [diff] [review]
no recursion in Traits::resolveSignatures() (2nd try)

The commented-out assertion in resolveSignatures can go.  (I know, it was like that before.)

In the second loop in resolveSignatures the test for t != NULL is redundant because t = *st which is already checked for NULL by the loop condition.

Comment 10

[Edwin Smith]

pushed http://hg.mozilla.org/tamarin-redux/rev/da0ad0dcd3e4

Comment 11

[Chris Peyer]

Engineering work item.  Marking as verified.