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

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jesse Ruderman]

Attachment: testcase (asserts fatally when loaded)

###!!! ABORT: Attempting to make self-dependent instance time: '!mEnd || aEnd.GetBaseTime() != mEnd', file content/smil/nsSMILInterval.cpp, line 134

Comment 1

[Jesse Ruderman]

Might depend on GC timing.

Comment 2

[Jesse Ruderman]

Attachment: stack trace

Comment 3

[Brian Birtles (:birtles)]

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.

Comment 4

[Jonathan Watt [:jwatt]]

Assigning to Brian at the moment so it doesn't fall off the radar.

Comment 5

[Brian Birtles (:birtles)]

Attachment: Patch v1a

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

Comment 6

[Mozilla RelEng Bot]

Try run for a53d59484ede is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=a53d59484ede
Results (out of 214 total builds):
    exception: 1
    success: 194
    warnings: 17
    failure: 2
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-a53d59484ede

Comment 7

[Daniel Holbert [:dholbert]]

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.

Comment 8

[Brian Birtles (:birtles)]

Thanks Daniel!

Pushed with review feedback addressed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc82c3485d1a

Comment 9

[Ed Morley [:emorley]]

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