Last Comment Bug 690994 - "ABORT: Attempting to make self-dependent instance time"
: "ABORT: Attempting to make self-dependent instance time"
Status: RESOLVED FIXED
: assertion, testcase
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- critical (vote)
: mozilla11
Assigned To: Brian Birtles (:birtles)
:
Mentors:
Depends on:
Blocks: stirdom
  Show dependency treegraph
 
Reported: 2011-09-30 20:17 PDT by Jesse Ruderman
Modified: 2012-02-01 13:56 PST (History)
3 users (show)
bbirtles: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (asserts fatally when loaded) (331 bytes, image/svg+xml)
2011-09-30 20:17 PDT, Jesse Ruderman
no flags Details
stack trace (2.59 KB, text/plain)
2011-09-30 20:18 PDT, Jesse Ruderman
no flags Details
Patch v1a (7.88 KB, patch)
2011-11-09 22:51 PST, Brian Birtles (:birtles)
dholbert: review+
Details | Diff | Review

Description Jesse Ruderman 2011-09-30 20:17:18 PDT
Created attachment 563925 [details]
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 2011-09-30 20:17:53 PDT
Might depend on GC timing.
Comment 2 Jesse Ruderman 2011-09-30 20:18:21 PDT
Created attachment 563926 [details]
stack trace
Comment 3 Brian Birtles (:birtles) 2011-10-01 03:46:11 PDT
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] (Away Jun. 27 - Jul. 13) 2011-10-10 00:45:37 PDT
Assigning to Brian at the moment so it doesn't fall off the radar.
Comment 5 Brian Birtles (:birtles) 2011-11-09 22:51:48 PST
Created attachment 573428 [details] [diff] [review]
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 2011-11-10 03:20:23 PST
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/bbirtles@mozilla.com-a53d59484ede
Comment 7 Daniel Holbert [:dholbert] (largely AFK until June 28) 2011-11-13 13:22:04 PST
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) 2011-11-13 20:03:15 PST
Thanks Daniel!

Pushed with review feedback addressed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc82c3485d1a
Comment 9 Ed Morley [:emorley] 2011-11-14 19:37:59 PST
https://hg.mozilla.org/mozilla-central/rev/dc82c3485d1a

Note You need to log in before you can comment on or make changes to this bug.