Closed Bug 995410 Opened 10 years ago Closed 10 years ago

reftest harness should call shouldSnapshotWholePage rather than testing for its existence

Categories

(Testing :: Reftest, defect, P1)

Product:
Testing
Component:
Reftest
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla31

People

(Reporter: dbaron, Assigned: dbaron)

References

Details

Attachments

(2 files)

stack showing where the translation gets blown away
3.04 KB, text/plain
Details
Call shouldSnapshotWholePage rather than testing for its existence
1.21 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review 
Robert O'Callahan wrote in bug 932233 comment #334:
> We get into a loop of endless whole-canvas invalidation. With some logging
> it shouldn't be that hard to figure out what causes that.
> 
> This code doesn't look right:
> http://dxr.mozilla.org/mozilla-central/source/layout/tools/reftest/reftest-
> content.js#918
>     if (shouldSnapshotWholePage) {
> I think we should actually be calling that function instead of testing
> whether it's defined :-)

Robert O'Callahan wrote in bug 932233 comment #335:
> That was added in bug 876626 by Matt. Matt, see previous comment ... are you
> interested in fixing that? :-)
> 
> With that fixed, we'll probably still have an infinite invalidation loop,
> but we'd be able to see which rects are being invalidated which would help
> narrow the problem down.

I attached a patch in attachment 8405505 [details] [diff] [review], and did a try run:

David Baron wrote in bug 932233 comment #340:
> https://tbpl.mozilla.org/?tree=Try&rev=e6e7a335235e

This try run shows we've regressed on at least some platforms (still waiting for full results).
The failures seem to be related to tests having a transparent backdrop instead of the white that was requested in the drawWindow calls.
Er, and on some platforms we just seem to be failing to offset the view into what we're painting -- in other words, a drawWindow that's not at (0,0) seems to be painting the stuff that is at (0, 0) at the location specified, rather than the stuff at that location.
So far the change I'm most suspicious of for having led to these failures is https://hg.mozilla.org/mozilla-central/rev/51dda2544c24 , but so far I'm mostly just guessing and haven't tested much.
I can repro the problem locally by changing this line:
https://hg.mozilla.org/mozilla-central/file/6149db60c6cb/content/canvas/src/CanvasRenderingContext2D.cpp#l3640
to false, so I think that is indeed the problem.
Assignee: nobody → dbaron
I'm not sure I'm reproducing *all* the problem locally, though, but I've fixed the half that I could reproduce (OP_OVER instead of OP_SOURCE), and have a mochitest for it.  I think there's a translation piece that I haven't reproduced yet, though.
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #5)
> I'm not sure I'm reproducing *all* the problem locally, though, but I've
> fixed the half that I could reproduce (OP_OVER instead of OP_SOURCE), and
> have a mochitest for it.

The OP_OVER patch (related to the condition from comment 4) did fix the failures on non-accelerated Windows (WinXP R, Win7/8 Ru), but didn't fix the Android and B2G failures.

> I think there's a translation piece that I haven't
> reproduced yet, though.

mattwoodrow suggests maybe:
[2014-04-11 22:03:20 -0700] <mattwoodrow> something to do with skia-gl (only enabled on android/b2g)
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #8)
> A try push to see if the failures still happen with the canvas backend
> switched from skia to cairo:
> 
> https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/0dc5940a187f/temp-canvas-no-skia
> https://tbpl.mozilla.org/?tree=Try&rev=0932e8e8205e

... which shows the problem seems to be specific to skia canvas (and maybe skia-gl, since I can't repro locally on Linux by setting just the skia canvas pref).
The try run in comment 10 says skia-gl canvas is not required (but skia canvas is, from comment 9).

The try run in comment 11 says my mochitest still doesn't cover the problem.
I split the OP_SOURCE -> OP_OVER fix, and related tests, into bug 995661.  The translation problem on Android and B2G still blocks enabling this, and I've given up (for now, anyway) on trying to get the B2G emulator to work.

Maybe Matt can look into the translation issue?
Assignee: dbaron → nobody
Flags: needinfo?(matt.woodrow)
One more try push, actually:
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/449b4cb3e3c6/test-patch-remove-translation
https://tbpl.mozilla.org/?tree=Try&rev=c80645d57b55
to test to see if the failures are the same if I just remove the Translate call that appears not to be doing anything in the cases where things are going wrong.
At first glance, the failures look the same with that Translate call removed, so it seems like the problem is that that Translate call isn't working.
Since another thought occurred to me, on more try run, with a different test patch:
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/6d1f033013c2/temp-canvas-no-skia
https://tbpl.mozilla.org/?tree=Try&rev=5dab8fe955dd
to see whether the skia canvas vs. cairo canvas difference is really just about which codepath is taken in CanvasRenderingContext2D::DrawWindow.  (I suspect it is.  If so, it seems worth testing if any other platforms have the same bug if forced through that codepath.)
So, indeed, the only reason the skia canvas backend is required is to trigger the codepath in DrawWindow that creates a new DrawTarget; if I force that codepath the failures still happen with the cairo canvas backend.
Oh, and the condition under which I can repro the translation problem locally is having OMTC enabled, which I hadn't done correctly before (though I thought I had).
The inverse translation happens to be living on the context when we blow it away here; removing the cairo_identity_matrix(mContext) call seems to fix the problem.
Flags: needinfo?(matt.woodrow)
I split the translation problem into bug 995721.
And hopefully a final try run with all three bugs, and new mochitests:
https://tbpl.mozilla.org/?tree=Try&rev=ef0b8637f70e
This is clearly what was intended when this code was introduced in bug
876626.
Attachment #8405929 - Flags: review?(matt.woodrow)
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
Attachment #8405929 - Flags: review?(matt.woodrow) → review+
https://hg.mozilla.org/mozilla-central/rev/33d397b3b008
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: