Closed Bug 624279 Opened 14 years ago Closed 14 years ago

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

Categories

(Testing :: Reftest, defect)

Product:
Testing
Component:
Reftest
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(blocking2.0 final+)

Status:
RESOLVED FIXED
Milestone:
mozilla2.0b12
Tracking Flags:
Tracking Status
blocking2.0 --- final+

People

(Reporter: dbaron, Assigned: dbaron)

Details

(Whiteboard: [softblocker])

Attachments

(2 files)

fix
4.03 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
patch
7.03 KB, patch
roc
: review+
Details | Diff | Splinter Review
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+
Attached patch fix — — Splinter 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: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b12
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: