Closed Bug 1318947 Opened 7 years ago Closed 6 years ago

Intermittent test_bug598895.html | Snapshot canvases are not the same size - comparing them makes no sense

Categories

(Core :: DOM: Navigation, defect)

Product:
Core
Component:
DOM: Navigation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: aryx, Assigned: milan)

Details

(Keywords: intermittent-failure, Whiteboard: [stockwell unknown])

Attachments

(1 file, 1 obsolete file)

More diagnostic information when the comparison fails. Carry r=jwatt
1.22 KB, patch
milan
: review+
Details | Diff | Splinter Review 
09:42:42     INFO - TEST-START | docshell/test/test_bug598895.html
09:42:43     INFO - TEST-INFO | started process screenshot
09:42:43     INFO - TEST-INFO | screenshot: exit 0
09:42:43     INFO - Buffered messages logged at 09:42:43
09:42:43     INFO - TEST-PASS | docshell/test/test_bug598895.html | Message should be 'loaded' 
09:42:43     INFO - TEST-PASS | docshell/test/test_bug598895.html | Message should be 'loaded' 
09:42:43     INFO - TEST-PASS | docshell/test/test_bug598895.html | Popups should look identical 
09:42:43     INFO - Buffered messages finished
09:42:43     INFO - TEST-UNEXPECTED-FAIL | docshell/test/test_bug598895.html | Snapshot canvases are not the same size - comparing them makes no sense 
09:42:43     INFO -     compareSnapshots@SimpleTest/WindowSnapshot.js:24:5
09:42:43     INFO -     window.onmessage@docshell/test/test_bug598895.html:40:8
09:42:43     INFO -     EventHandlerNonNull*@docshell/test/test_bug598895.html:30:1
09:42:43     INFO -     SimpleTest._newCallStack/rval@SimpleTest/SimpleTest.js:146:17
09:42:43     INFO -     EventHandlerNonNull*this.addLoadEvent@SimpleTest/SimpleTest.js:171:13
09:42:43     INFO -     @SimpleTest/SimpleTest.js:1372:5
09:42:43     INFO - Not taking screenshot here: see the one that was previously logged
09:42:43     INFO - TEST-UNEXPECTED-FAIL | docshell/test/test_bug598895.html | Popups should not look identical 
09:42:43     INFO -     window.onmessage@docshell/test/test_bug598895.html:40:5
09:42:43     INFO -     EventHandlerNonNull*@docshell/test/test_bug598895.html:30:1
09:42:43     INFO -     SimpleTest._newCallStack/rval@SimpleTest/SimpleTest.js:146:17
09:42:43     INFO -     EventHandlerNonNull*this.addLoadEvent@SimpleTest/SimpleTest.js:171:13
09:42:43     INFO -     @SimpleTest/SimpleTest.js:1372:5
Component: Hardware Abstraction Layer (HAL) → Document Navigation
There are 47 failures in the past week. All occurrences were on linux64-qr opt.
Log failure example: https://treeherder.mozilla.org/logviewer.html#?repo=mozilla-inbound&job_id=173104494&lineNumber=3524

Relevant part of the log: 
[task 2018-04-11T15:02:41.740Z] 15:02:41     INFO - Buffered messages finished
[task 2018-04-11T15:02:41.742Z] 15:02:41     INFO - TEST-UNEXPECTED-FAIL | docshell/test/test_bug598895.html | Snapshot canvases are not the same size - comparing them makes no sense 
[task 2018-04-11T15:02:41.742Z] 15:02:41     INFO -     compareSnapshots@SimpleTest/WindowSnapshot.js:24:5
[task 2018-04-11T15:02:41.742Z] 15:02:41     INFO -     window.onmessage@docshell/test/test_bug598895.html:40:8
[task 2018-04-11T15:02:41.742Z] 15:02:41     INFO -     EventHandlerNonNull*@docshell/test/test_bug598895.html:30:1
[task 2018-04-11T15:02:41.743Z] 15:02:41     INFO -     rval@SimpleTest/SimpleTest.js:146:17
[task 2018-04-11T15:02:41.744Z] 15:02:41     INFO -     EventHandlerNonNull*this.addLoadEvent@SimpleTest/SimpleTest.js:171:13
[task 2018-04-11T15:02:41.745Z] 15:02:41     INFO -     @SimpleTest/SimpleTest.js:1444:5
[task 2018-04-11T15:02:41.745Z] 15:02:41     INFO - Not taking screenshot here: see the one that was previously logged
[task 2018-04-11T15:02:41.745Z] 15:02:41     INFO - TEST-UNEXPECTED-FAIL | docshell/test/test_bug598895.html | Popups should not look identical 
[task 2018-04-11T15:02:41.745Z] 15:02:41     INFO -     window.onmessage@docshell/test/test_bug598895.html:40:5
[task 2018-04-11T15:02:41.747Z] 15:02:41     INFO -     EventHandlerNonNull*@docshell/test/test_bug598895.html:30:1
[task 2018-04-11T15:02:41.747Z] 15:02:41     INFO -     rval@SimpleTest/SimpleTest.js:146:17
[task 2018-04-11T15:02:41.747Z] 15:02:41     INFO -     EventHandlerNonNull*this.addLoadEvent@SimpleTest/SimpleTest.js:171:13
[task 2018-04-11T15:02:41.748Z] 15:02:41     INFO -     @SimpleTest/SimpleTest.js:1444:5

:overholt Can you please check this?
Flags: needinfo?(overholt)
Henri, do you think this is related to bug 598895 or bug 637644?
Flags: needinfo?(overholt) → needinfo?(hsivonen)
(In reply to Andrew Overholt [:overholt] from comment #25)
> Henri, do you think this is related to bug 598895 or bug 637644?

Seems unlikely to me. My guess from the error message is that we have a timing problem with a newly-opened window getting resized to its intended size and the test code taking the snapshot.

The recent failures have all related to Quantum Render, so maybe we should ask the graphics team what is being changed about how windows are sized upon opening relative to letting the child process believe that the opening has completed.
Flags: needinfo?(hsivonen) → needinfo?(milan)
Comment on attachment 8969443 [details]
Bug 1318947: If expecting snapshots to be different, having them be different size should be considered a succuess. Add more diagnostic information when snapshot comparison fails.

https://reviewboard.mozilla.org/r/238206/#review243924

The extra diagnostic information seems good. The change to use `expectEqual` seems off though. This is happening intermittently, and it sounds like there's a race. If the change to use `expectEqual` "helps" here, it seems like that's just a coincidence rather than that being because this is the correct logic.
(In reply to Jonathan Watt [:jwatt] from comment #28)
>...
> 
> The extra diagnostic information seems good. The change to use `expectEqual`
> seems off though. This is happening intermittently, and it sounds like
> there's a race. If the change to use `expectEqual` "helps" here, it seems
> like that's just a coincidence rather than that being because this is the
> correct logic.

I was just about to comment to that effect.  I am more interested in the diagnostic part and given the intermittent nature, if this makes the problem go away, it will be because sometimes we have the differing canvases be different size, and sometimes be the same size, but different content.

Now, in general though, do we want to return true when we expect a difference, and sizes are different?  That used to be the case before bug 1089753 changed it to always fail when the sizes are different, even if we want them to be different.
Flags: needinfo?(milan) → needinfo?(jwatt)
The change in bug 1089753 is four years old, so clearly isn't responsible for this error, more of a question as to what we should be testing.
In the last 6 days there are no failures regarding this bug.
Seems that somehow it was fixed
Whiteboard: [stockwell disable-recommended] → [stockwell unknown]
Comment on attachment 8969443 [details]
Bug 1318947: If expecting snapshots to be different, having them be different size should be considered a succuess. Add more diagnostic information when snapshot comparison fails.

https://reviewboard.mozilla.org/r/238206/#review246674

::: testing/mochitest/tests/SimpleTest/WindowSnapshot.js:24
(Diff revision 1)
>  // If the two snapshots don't compare as expected (true for equal, false for
>  // unequal), returns their serializations as data URIs.  In all cases, returns
>  // whether the comparison was as expected.
>  function compareSnapshots(s1, s2, expectEqual, fuzz) {
>    if (s1.width != s2.width || s1.height != s2.height) {
> -    ok(false, "Snapshot canvases are not the same size - comparing them makes no sense");
> +    ok(!expectEqual, "Snapshot canvases are not the same size: " + s1.width + "x" + s1.height + " vs. " + s2.width + "x" + s2.height);

r+ to land the diagnostic changes, but not the changes in condition for now. I think we should consider changing the success condition separately, and only once we understand why the canvases may be different sizes (which we don't seem to right now).

Also update the commit message of course.
Attachment #8969443 - Flags: review?(jwatt) → review+
Flags: needinfo?(jwatt)
Attachment #8969443 - Attachment is obsolete: true
Assignee: nobody → milaninbugzilla
No recent failures, but update the diagnostic information so that we get more details if the failures restart.
Attachment #8973272 - Flags: review+
Pushed by cbrindusan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2eb5810dad5b
More diagnostic information when snapshot comparison fails. r=jwatt
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2eb5810dad5b
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.