384631 - reftest gets confused about which testcases it's comparing

Status: NEW

(Testing :: Reftest, defect)

Description

[Adam Guthrie]

Attached is a log snippet from one of the Linux tinderboxen that failed a reftest. If you compare the data: images, you'll see that it's not comparing the correct tests. The manifiest file is:

== 370629-1.html 370629-1-ref.html
== 370629-2.html 370629-2-ref.html
== 370692-1.xhtml 370692-1-ref.xhtml

and it seems to be comparing one of the files for bug 370629 with a file for bug 370692. It's like the URL isn't getting shifted correctly when a test fails to load.

Comment 1

[Adam Guthrie]

Attachment: tinderbox log snippet

Comment 2

[:Gavin Sharp [email: [e-mail]]]

I remember vlad and/or roc mentioning something like this on IRC a little while ago.

Comment 3

[Rob Campbell [:rc] (:robcee)]

Sounds like an issue with the vm not having enough memory for some of these large surfaces (based on discussion with bz in #developers). We'll track this and compare results with the replacement machine when it's available.

Comment 4

[Adam Guthrie]

I could see the OOM situation causing the testcase load failure, but comparing two unrelated testcases as a result seems like it's caused by a bug in the test harness itself.

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

The problem is that the reftest harness uses the same <canvas> for the entire run; ideally, it should create a new canvas of reach test.  The reason is that if an error in cairo is triggered, the canvas context is left in an error state, and all future drawing ops become noops.  ToDataURL doesn't check for error, so the net result is that we end up with all == tests passing after the error condition, and all != tests failing (because it's constantly getting the same image out).

Comment 6

[Rob Campbell [:rc] (:robcee)]

seems like it should be trivial to dispose of and recreate the <canvas> on each testrun. Anyone feel like taking a crack at it?

Comment 7

[Adam Guthrie]

The thing is, we don't pass all the == test and fail all the != tests after this happens. We only fail the one right after the "REFTEST UNEXPECTED FAIL (LOADING)" test and seem to continue on just fine after that...

Also, should there be a bug filed about canvas's ToDataURL impl. not checking for error state of the canvas or is that just this? http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/content/html/content/src/nsHTMLCanvasElement.cpp&rev=3.17&mark=377-378#372

Comment 8

[Jonathan Watt [:jwatt]]

Attachment: address comment 5

Comment 9

[Jonathan Watt [:jwatt]]

ispiked has pointed out vlad's patch on bug 387132. not sure what should happen with this patch in light of that. I was just trying to quickly help with the post cairo update orange, I don't mind really.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So I was waiting to review this until bug 387132 landed -- any chance you could merge this with that and attach a new patch?

Comment 11

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

I'm not actually sure what the current status is with bug 387132 -- the reftest-fast module is still disabled, but the other code (in reftest.js etc.) is there.  It just falls back to doing data URL comparisons if it can't obtain the component.  But the code is different enough that a new patch for this would be needed (probably by pulling out canvas-creation into some kind of recreateCanvas() function that's called at the start of each reftest).

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 273633 [details] [diff] [review]
address comment 5

Cancelling review request since a new patch is needed.