Last Comment Bug 670452 - Test for bug 113934 fails to complete due to JS error
: Test for bug 113934 fails to complete due to JS error
Status: RESOLVED FIXED
[qa-]
: regression
Product: Core
Classification: Components
Component: Document Navigation (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: mozilla8
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
:
: Andrew Overholt [:overholt]
Mentors:
: 670838 (view as bug list)
Depends on:
Blocks: 552605 652494
  Show dependency treegraph
 
Reported: 2011-07-09 11:46 PDT by Graeme McCutcheon [:graememcc]
Modified: 2011-10-20 22:31 PDT (History)
9 users (show)
jruderman: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
fixed


Attachments
Fix (1.53 KB, patch)
2011-07-12 06:46 PDT, Boris Zbarsky [:bz] (still a bit busy)
joe: review+
asa: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Graeme McCutcheon [:graememcc] 2011-07-09 11:46:46 PDT
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
Comment 1 Boris Zbarsky [:bz] (still a bit busy) 2011-07-09 20:34:22 PDT
And of course someone made exceptions non-fatal in chrome tests.  :(

Joe, can you take a look at this please?
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2011-07-09 20:38:11 PDT
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?
Comment 3 Boris Zbarsky [:bz] (still a bit busy) 2011-07-09 20:38:49 PDT
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.
Comment 4 Graeme McCutcheon [:graememcc] 2011-07-10 03:55:27 PDT
> 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.
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2011-07-10 08:25:37 PDT
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.
Comment 6 Boris Zbarsky [:bz] (still a bit busy) 2011-07-11 21:38:32 PDT
*** Bug 670838 has been marked as a duplicate of this bug. ***
Comment 7 Cameron McCormack (:heycam) 2011-07-11 22:00:18 PDT
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?
Comment 8 Boris Zbarsky [:bz] (still a bit busy) 2011-07-12 05:46:13 PDT
The 1200 and 120 look correct.

The GetSize results there look moderately bogus to me.
Comment 9 Joe Drew (not getting mail) 2011-07-12 06:34:51 PDT
Is there anything I can do for this?
Comment 10 Boris Zbarsky [:bz] (still a bit busy) 2011-07-12 06:46:28 PDT
Created attachment 545370 [details] [diff] [review]
Fix

Sure, review the patch!
Comment 11 Joe Drew (not getting mail) 2011-07-12 07:32:37 PDT
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.
Comment 12 Boris Zbarsky [:bz] (still a bit busy) 2011-07-12 09:09:52 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/7dd3e78e88ed
Comment 13 Boris Zbarsky [:bz] (still a bit busy) 2011-07-13 08:44:12 PDT
http://hg.mozilla.org/releases/mozilla-aurora/rev/d5b0d70cf3bf
Comment 15 Boris Zbarsky [:bz] (still a bit busy) 2011-07-13 09:19:25 PDT
Actually, we have no test to make sure this doesn't regress... that's what the "in-testsuite?" is about.
Comment 16 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-09-22 15:55:29 PDT
qa- as no QA fix verification necessary
Comment 17 Jesse Ruderman 2011-10-20 22:31:59 PDT
Bug 652494 covers fixing the test suite. Removing this from the list of bugs that need tests created.

Note You need to log in before you can comment on or make changes to this bug.