"load" reftests with reftest-print are no longer testing printing

RESOLVED FIXED in mozilla2.0b12

Status

defect
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: dbaron, Assigned: dbaron)

Tracking

Trunk
mozilla2.0b12
Points:
---

Firefox Tracking Flags

(blocking2.0 final+)

Details

(Whiteboard: [softblocker])

Attachments

(2 attachments)

I just wrote a crashtest for bug 595740, with <html class="reftest-print">.  Without the patch to fix the bug, it shows the bug when I run it as:
  != 595740-1.html about:blank
but not when I load it as:
  load 595740-1.html

I suspect recent changes to the reftest harness mean that we're no longer running print reftests properly for "load" tests.

If this is a general problem, this means we're missing a *lot* of our printing test coverage.
blocking2.0: --- → final+
Posted patch fixSplinter Review
For me, I still get a crash on trunk, but after the test has reported successfully loading. Maybe that's just luck.

This patch reverts to the old behavior of adding reftest-print while 'load' is firing. For me, this makes the crash occur before the test reports success.
Attachment #502452 - Flags: review?(dbaron)
Comment on attachment 502452 [details] [diff] [review]
fix

r=dbaron
Attachment #502452 - Flags: review?(dbaron) → review+
(In reply to comment #1)
> Created attachment 502452 [details] [diff] [review]
> fix
> 
> For me, I still get a crash on trunk, but after the test has reported
> successfully loading. Maybe that's just luck.

I was running it as the only file in the manifest; that might make a difference.

I'll try testing this later.
Keywords: checkin-needed
Whiteboard: [needs review] → [needs landing]
Whiteboard: [needs landing] → [needs landing][softblocker]
I just pushed this to try:
http://tbpl.mozilla.org/?tree=MozillaTry&rev=6aed9d7821f1


Then I tested it against the original problem I reported, which it turns out it doesn't fix.

In particular, the steps I'm testing are:
 1. revert http://hg.mozilla.org/mozilla-central/rev/e7504a9cf0cb
 2. replace layout/generic/crashtests/crashtests.list with the single line:
load 595740-1.html
    (which changes that line from != to load, and removes all the rest)
 3. run "TEST_PATH=layout/generic/crashtests/crashtests.list make crashtest" in the objdir

Both with and without this reftest harness patch, I don't get a crash.  But, as described in comment 0, if I change the "load 595740-1.html" to "!= 595740-1.html about:blank" then I get a crash.
(For the record, the try run was fine.)
Sigh, pushed in http://hg.mozilla.org/mozilla-central/rev/002056f953c7 because I didn't realize I hadn't cc'ed myself so I didn't see those comments, I just looked at the try run.
Keywords: checkin-needed
Whiteboard: [needs landing][softblocker] → [softblocker]
(In reply to comment #4)
> In particular, the steps I'm testing are:
>  1. revert http://hg.mozilla.org/mozilla-central/rev/e7504a9cf0cb
>  2. replace layout/generic/crashtests/crashtests.list with the single line:
> load 595740-1.html
>     (which changes that line from != to load, and removes all the rest)
>  3. run "TEST_PATH=layout/generic/crashtests/crashtests.list make crashtest" in
> the objdir
> 
> Both with and without this reftest harness patch, I don't get a crash.  But, as
> described in comment 0, if I change the "load 595740-1.html" to "!=
> 595740-1.html about:blank" then I get a crash.

I get a crash with either "!=" or "load", on Windows. I assume you were testing on Linux? I'll try Linux.
Same on Linux.

This is with a debug build. Did you test with debug or opt build? Hopefully it doesn't matter...
I'm testing with debug+opt, as usual. :-)
I can make an opt build, but it'll take a little while since I'm in the middle of other stuff, and I suspect it won't show the problem anyway --- presumably opt builds should crash the same way, and it's unlikely the reftest harness is affected by opt. Maybe you could debug this?
(In reply to comment #4)
>  2. replace layout/generic/crashtests/crashtests.list with the single line:
> load 595740-1.html
>     (which changes that line from != to load, and removes all the rest)

It's important that 595740-1.html is the last test run.  If there's another test after it, we get the crash, but the harness associates the crash with the following test.
http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/db2ed335cb98/reftest-print-paint fixes the problem locally for me.  The problem seems to be that when we go directly from AfterOnLoadScripts into RecordResult, and InitializeCurrentCanvasWithSnapshot doesn't do any painting, nothing forces us to do any painting.  This patch makes us wait until painting happens in that case.

Note that it relies on the fact that painting flushes layout... I think that's still true, anyway.

As I noted in the patch description, it was tempting to do a bunch of cleanup, but I didn't.


I just pushed this to try.
...where it seems to have caused some crashtests that remove the root element from the document to time out, because we get:

REFTEST INFO | flushWindow failed: TypeError: win.document.documentElement is null

JavaScript error: chrome://reftest/content/reftest.js, line 1081: contentRootElement is null

line 1081 is:

            contentRootElement.dispatchEvent(notification);
http://hg.mozilla.org/mozilla-central/rev/95b2986b495e
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b12
You need to log in before you can comment on or make changes to this bug.