Closed Bug 690994 Opened 12 years ago Closed 12 years ago

"ABORT: Attempting to make self-dependent instance time"


(Core :: SVG, defect)

Not set





(Reporter: jruderman, Assigned: birtles)



(Keywords: assertion, testcase)


(3 files)

###!!! ABORT: Attempting to make self-dependent instance time: '!mEnd || aEnd.GetBaseTime() != mEnd', file content/smil/nsSMILInterval.cpp, line 134
Might depend on GC timing.
Attached file stack trace
This will almost certainly be my fault, but I'm away for the next month. If someone else feels brave, feel free to take it or otherwise assign it to me.
Assigning to Brian at the moment so it doesn't fall off the radar.
Assignee: nobody → birtles
Attached patch Patch v1aSplinter Review
Proposed patch.

The root problem was that we were testing for self-referential end times in the general case, but not in the specific case where we detected two zero-duration intervals in a row. All the fix really requires is to move the zero-duration checking inside the loop where we check for self-referential times.

However, in trying to fix this I realised that this code is not very clear and not strictly correct either so I made the following changes:
* Move the comment explaining the do...while loops next to the 'while' condition since generally that makes a lot more sense
* Change the check for coincident zero-duration intervals to tempEnd->Time() == beginAfter which is basically a simplification of:
   tempBegin->Time() == tempEnd->Time &&
   tempBegin->Time() == beginAfter
(beginAfter is generally the end of the previous interval. In the couple of cases where it's not, prevIntervalWasZeroDur is set to false.)
* This second change also allowed removing some code at the start of the method (which was confusing and possibly wrong)
* Other tweaks to comments and code to make it easier to read
Attachment #573428 - Flags: review?(dholbert)
Try run for a53d59484ede is complete.
Detailed breakdown of the results available here:
Results (out of 214 total builds):
    exception: 1
    success: 194
    warnings: 17
    failure: 2
Builds available at
Comment on attachment 573428 [details] [diff] [review]
Patch v1a

Looks good -- just some comment nits:

>+        // SMIL doesn't allow for coincident zero-duration intervals so if the
>+        // previous interval was zero-duration, and tempEnd is going to give us
>+        // another zero duration interval then look for another end to use
>+        // instead.

Readability nit: add comma before "so if the" and "then look for".  (makes it easier to read, for me at least. :))

>+    // When we choose the interval endpoints we don't allow coincident
>+    // zero-duration intervals so if we arrive here and we have a zero-duration
>+    // interval starting at the same point as a previous zero-duration interval
>+    // then it must be because we've applied constraints to the active duration.
>+    // In that case we will potentially run into an infinite loop so we break it
>+    // by searching for the next interval that starts AFTER our current
>+    // zero-duration interval.
>+    if (prevIntervalWasZeroDur && tempEnd->Time() == beginAfter) {

This comment is a bit complex & would also get a readability-win from some commas. :) 

In particular, seems like we could use a comma before "we don't allow coincident", "so if we arrive", "then it must be", and "so we break it".

r=me with that.
Attachment #573428 - Flags: review?(dholbert) → review+
Thanks Daniel!

Pushed with review feedback addressed:
OS: Mac OS X → All
Hardware: x86_64 → All
Whiteboard: [inbound]
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Flags: in-testsuite+
Whiteboard: [inbound]
You need to log in before you can comment on or make changes to this bug.