Test for bug 113934 fails to complete due to JS error

RESOLVED FIXED in Firefox 7

Status

()

Core
Document Navigation
P1
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: graememcc, Assigned: bz)

Tracking

({regression})

Trunk
mozilla8
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox7- fixed)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
Running mochitest-chrome today, I noticed a couple of windows from the tests for bug 113934 hanging around until the end of the test suite. Handily, I had an older tree lying around to confirm this was new. Same when running the test in isolation.

We're now only running 3 tests out of 13, before hitting an uncaught JS exception - an NS_ERROR_FAILURE bubbling up from the call to compareCanvases in compareSnapshots.

hg bisect points to http://hg.mozilla.org/mozilla-central/rev/39bd0f6314c3 from bug 552605.

This isn't just a local problem, the slaves running m-o on tinderbox are hitting this too (at least linux debug, not checked opt or other platforms). 

Build logs before/after the push:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1309594430.1309598240.28946.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1309600613.1309605277.30375.gz

For the record, compareCanvases bales when checking the image sizes are equal, but it's presumably the change to WindowSnapshot in the cset above that's done the damage
And of course someone made exceptions non-fatal in chrome tests.  :(

Joe, can you take a look at this please?
Blocks: 552605
tracking-firefox7: --- → ?
Oh, I see, we used to never actually take the gWindowUtils codepath?

Is there a reason that throws on a size mismatch instead of just returning false?

Graeme, does making that change fix things here?
Oh, and I requested tracking because we're missing part of our test suite now, unless either this bug is fixed or the change to WindowSnapshot is backed out.
(Reporter)

Comment 4

6 years ago
> Oh, I see, we used to never actually take the gWindowUtils codepath?
Yes, using an older tree, for this test at least, the comparison was only on the data:// URLs.
 
> Is there a reason that throws on a size mismatch instead of just returning
> false?
Looking at bug 387132, compareCanvases returns the number of differing pixels rather than true/false, so differing sizes would make that number meaningless.
OK.  Does changing WindowSnapshot to check the imagedata sizes itself on that codepath and treat the result as "not equal" fix things?  I can post a patch for that tomorrow if you'd prefer.
Duplicate of this bug: 670838
These are the actual values being compared:

(gdb) p (img1.mRawPtr)->GetSize()
$5 = {
  <mozilla::gfx::BaseSize<int,nsIntSize>> = {
    width = 1606389496, 
    height = 32767
  }, <No data fields>}
(gdb) p (img2.mRawPtr)->GetSize()
$6 = {
  <mozilla::gfx::BaseSize<int,nsIntSize>> = {
    width = 1606389496, 
    height = 32767
  }, <No data fields>}
(gdb) p (img1.mRawPtr)->Stride()
$7 = 1200
(gdb) p (img2.mRawPtr)->Stride()
$8 = 120

Are they sensible?
Blocks: 652494
The 1200 and 120 look correct.

The GetSize results there look moderately bogus to me.
Is there anything I can do for this?
Created attachment 545370 [details] [diff] [review]
Fix

Sure, review the patch!
Attachment #545370 - Flags: review?(joe)
Assignee: nobody → bzbarsky
Priority: -- → P1
Whiteboard: [need review]
Comment on attachment 545370 [details] [diff] [review]
Fix

It feels weird to be calling ok() from this function, but as long as it works I'm happy.
Attachment #545370 - Flags: review?(joe) → review+
http://hg.mozilla.org/integration/mozilla-inbound/rev/7dd3e78e88ed
Flags: in-testsuite?
Whiteboard: [need review]
Target Milestone: --- → mozilla8

Updated

6 years ago
Attachment #545370 - Flags: approval-mozilla-aurora+

Updated

6 years ago
tracking-firefox7: ? → -
http://hg.mozilla.org/releases/mozilla-aurora/rev/d5b0d70cf3bf
status-firefox7: --- → fixed
http://hg.mozilla.org/mozilla-central/rev/7dd3e78e88ed
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Actually, we have no test to make sure this doesn't regress... that's what the "in-testsuite?" is about.
Flags: in-testsuite+ → in-testsuite?
qa- as no QA fix verification necessary
Whiteboard: [qa-]

Comment 17

6 years ago
Bug 652494 covers fixing the test suite. Removing this from the list of bugs that need tests created.
Flags: in-testsuite? → in-testsuite-
You need to log in before you can comment on or make changes to this bug.