Many B2G reftests fail after enabling OOP, because partial drawWindow calls are drawing too small an area

RESOLVED FIXED in Firefox 32

Status

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: vichen, Assigned: u459114)

Tracking

unspecified
mozilla33
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox31 unaffected, firefox32 fixed, firefox33 fixed, b2g-v1.4 unaffected, b2g-v2.0 fixed, b2g-v2.1 fixed)

Details

Attachments

(4 attachments, 21 obsolete attachments)

112.82 KB, image/png
Details
8.10 KB, image/png
Details
1.35 KB, patch
u459114
: review+
Details | Diff | Splinter Review
238.55 KB, patch
u459114
: review+
Details | Diff | Splinter Review
Reporter

Description

5 years ago
Follow: Bug 922680 Comment 106
After enable OOP on B2G reftest, there are many reftests fail. After regress among patches (http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5405d6f4e3c6&tochange=92dd49bd29b6), the fails is between the two patches.

Fail: Apr 15 13:08:13 c7c4c0d0f613 https://tbpl.mozilla.org/?tree=Try&rev=d823e000d59c
Pass: Apr 15 13:08:13 92dd49bd29b6 https://tbpl.mozilla.org/?tree=Try&rev=d097b88f2b72 

Please focus on R4-R15
Reporter

Comment 1

5 years ago
dbaron,
As the patches between d823e000d59c and d097b88f2b72 are submitted by you, would please check first.
Flags: needinfo?(dbaron)
Reporter

Updated

5 years ago
Blocks: 922680
Reporter

Comment 2

5 years ago
(In reply to Vincent Chen [:vichen] from comment #1)
> dbaron,
> As the patches between d823e000d59c and d097b88f2b72 are submitted by you,
> would please check first.

Update: patches between c7c4c0d0f613 and 92dd49bd29b6
Those patches re-enabled a feature of the reftest harness that was accidentally broken; see bug 995410.  With that bug fixed, the reftest harness again uses partial drawWindow calls to redraw only the invalid region, instead of redrawing the entire screen.

The other bugs in that range (bug 995661 and bug 995721) fixed bugs where partial-window drawWindow calls were broken.  It looks like this set of failures shows another way in which a partial window canvas drawWindow call is broken.  Many of the failures in comment 0's "Fail" line are the same tests that I saw on other platforms when testing bug 995410 without bug 995661 or bug 995721.

In this case, the problem looks somewhat different from bug 995661 and bug 995721, though.  It appears that the area being redrawn is slightly too small.  (layout/reftests/transform/dynamic-addremove-1a.html is perhaps a good example test to debug; it's one of the failures in R15.  It shows failures around the edge of the box that moved, and also around the edge of the window.)
Flags: needinfo?(dbaron)
Bug 1018381 seems to cover the same problem as this bug.
Also, bug 922680 comment 97 links to some useful history on how we got to this point, I think.
Summary: Many reftests fails after enable OOP → Many B2G reftests fail after enabling OOP, because partial drawWindow calls are drawing too small an area
Once this is fixed we should re-enable all the tests that were disabled by bug 981477 and re-triage them on try. To do that we can backout all the patches that landed in that bug, which in order they should be backed out in are:

https://hg.mozilla.org/mozilla-central/rev/e1d54d299ab3
https://hg.mozilla.org/mozilla-central/rev/d4d29af7b3d0
https://hg.mozilla.org/mozilla-central/rev/c4eebcb1c390
https://hg.mozilla.org/mozilla-central/rev/b36a923d9b4a
https://hg.mozilla.org/mozilla-central/rev/f05fbe6261ac
Assignee

Comment 8

5 years ago
Take it
Assignee: nobody → cku
Assignee

Comment 9

5 years ago
update what I found so far.

By looking over comment at bug 100722 and bug 1018381. Here are actions:
1. Remove all "random-if" failure type in tests/layout/reftests/flexbox
2. submit a try run
   Try result: https://tbpl.mozilla.org/?tree=Try&rev=f7c9eef86098
3. Many failures(because of #1). I think flexbox-dyn-changePadding-1a is 
   really trivial one. Look into this failure first.
   Pass criteria: == flexbox-dyn-changePadding-1a.xhtml flexbox-dyn-changePadding-1-ref.xhtml
4. Look into TEST-UNEXPECTED-FAIL of flexbox-dyn-changePadding-1a.xhtml
   https://tbpl.mozilla.org/php/getParsedLog.php?id=41871625&tree=Try&full=1#error4

   From the log:
   06:44:40     INFO -  REFTEST INFO | Saved log: [CONTENT] Rect: 8 8 108 108
   
   I "think", if the value of Rect change to  {8, 8, 110, 110}, thing should goes right.
   There is no build env at home. Need to confirm this assumption after entering 
   office tomorrow.

   Rect value is set here:
   http://dxr.mozilla.org/mozilla-central/source/layout/tools/reftest/reftest-content.js#946
   Suspect: round problem here:
   http://dxr.mozilla.org/mozilla-central/source/layout/tools/reftest/reftest-content.js#942
Assignee

Comment 10

5 years ago
My description in comment 9 is not correct.
Size of <div id="tweakMe"> is (100, 100)

06:44:40     INFO -  REFTEST INFO | Saved log: DoDrawWindow 8,8,100,100
ctx.drawWindow(gContainingWindow,8,8,100,100,"rgb(255,255,255)", gDrawWindowFlags);

Size of this drawing is correct. Need look into gecko side to figure out why 2 pixels miss on right/ bottom boundary of <div id="tweakMe">
Assignee

Comment 11

5 years ago
peter,
since you are working on bug 846421, in which you have to trace into drawWindow call flow in gecko. Do you have any idea on this problem?
Flags: needinfo?(pchang)
Assignee

Comment 12

5 years ago
Posted image Diagram: 2 pixels border β€”
(In reply to C.J. Ku[:cjku] from comment #11)
> peter,
> since you are working on bug 846421, in which you have to trace into
> drawWindow call flow in gecko. Do you have any idea on this problem?

It would be good to dump the root frame during the drawWindow call.
You can define DEBUG_FRAME_DUMP from [1] and add "rootFrame->DumpFrameTree()" before [2].

[1]http://dxr.mozilla.org/mozilla-central/source/layout/generic/nsFrameList.h#16
[2]http://dxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp?from=nsPresShell.cpp&case=true#4836
Flags: needinfo?(pchang)
Assignee

Comment 15

5 years ago
Ahifting two pixels while drawWindow
  http://dxr.mozilla.org/mozilla-central/source/layout/tools/reftest/reftest.js#1454
  ctx.drawWindow(gContainingWindow, x + 2, y +2, w, h, "rgb(255,255,255)",

Before change
https://tbpl.mozilla.org/?tree=Try&rev=f7c9eef86098
R10 failed.

After change
https://tbpl.mozilla.org/?tree=Try&rev=4c9f1a75ec28
R10 pass.
Assignee

Comment 16

5 years ago
Assignee

Comment 17

5 years ago
Assignee

Comment 18

5 years ago
attachment 8441998 [details] [diff] [review]: remove random-if failure type for testing purpose
attachment 8442000 [details] [diff] [review]: set margin and padding value of the remote iframe as 0. And set border as none at the same time

Try result
https://tbpl.mozilla.org/?tree=Try&rev=bd09c215e547
Assignee

Comment 19

5 years ago
Correct typo
Attachment #8442000 - Attachment is obsolete: true
Assignee

Comment 21

5 years ago
Comment on attachment 8442002 [details] [diff] [review]
Part 2: Set margin/padding of remote iframe to 0

Hi dbaron,
I think the problem cause by padding of remote-iframe, which is created in OOP case only. 
After applying attachment 8441998 [details] [diff] [review] and attachment 8442002 [details] [diff] [review], all test cases in flexbox passed at local side.

Please help to review this patch, thanks
Attachment #8442002 - Flags: review?(dbaron)
Assignee

Updated

5 years ago
Attachment #8442002 - Attachment is patch: true
Assignee

Comment 23

5 years ago
(In reply to C.J. Ku[:cjku] from comment #22)
> Full try
> https://tbpl.mozilla.org/?tree=Try&rev=fe63872d22ce
Most Red color are caused by turning on frame tree dump
"Output exceeded 52428800 bytes, remaining output has been truncated (output was 52435769 bytes)"

Otherwise, test result looks good.
Assignee

Comment 24

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=fe63872d22ce

Orange in R12 is an new one after apply this patch.
Assignee

Comment 25

5 years ago
(In reply to C.J. Ku[:cjku] from comment #24)
> https://tbpl.mozilla.org/?tree=Try&rev=fe63872d22ce
> 
> Orange in R12 is an new one after apply this patch.

Looks like this patch make this fail-if test case pass.
fails-if(B2G) == menclose-2-circle.html menclose-2-circle-ref.html
INFO -  REFTEST TEST-UNEXPECTED-PASS | http://10.0.2.2:8888/tests/layout/reftests/mathml/menclose-2-circle.html | image comparison (==)
Comment on attachment 8442002 [details] [diff] [review]
Part 2: Set margin/padding of remote iframe to 0

r=dbaron.  Thanks.

(It seems a bit curious that this causes a problem only for the OOP case, though.)

Once this is landed, the random-if() and skip-if() annotations due to this bug should also be removed.
Attachment #8442002 - Flags: review?(dbaron) → review+
(In reply to David Baron [:dbaron] (busy May 31-June 15) (UTC-7) from comment #26)
> (It seems a bit curious that this causes a problem only for the OOP case,
> though.)

Perhaps that's because there's a bug in a codepath used by drawWindow that's incorrectly subtracting this offset?
Assignee

Comment 28

5 years ago
(In reply to David Baron [:dbaron] (busy May 31-June 15) (UTC-7) from comment #26)
> Comment on attachment 8442002 [details] [diff] [review]
> Part 2: Set margin/padding of remote iframe to 0
> 
> r=dbaron.  Thanks.
Will do. Thank you for reviewing
> (It seems a bit curious that this causes a problem only for the OOP case,
> though.)
> 
> Once this is landed, the random-if() and skip-if() annotations due to this
> bug should also be removed.
That's a thing that I am going to figure out.
I suspect that is because default css value of iframe and xul:browser is different 
OOP
  gBrowser = gContainingWindow.document.createElementNS(XHTML_NS, "iframe");
Non-OOP
  gBrowser = gContainingWindow.document.createElementNS(XUL_NS, "xul:browser");
Assignee

Comment 29

5 years ago
carry r+
Attachment #8441998 - Attachment is obsolete: true
Attachment #8442002 - Attachment is obsolete: true
Attachment #8442612 - Flags: review+
Assignee

Comment 30

5 years ago
Posted patch Part 2: backout e1d54d299ab3 (obsolete) β€” β€” Splinter Review
Assignee

Comment 31

5 years ago
Posted patch Part 3: backout d4d29af7b3d0 (obsolete) β€” β€” Splinter Review
Assignee

Comment 32

5 years ago
Posted patch Part 4: backout b36a923d9b4a (obsolete) β€” β€” Splinter Review
Assignee

Updated

5 years ago
Attachment #8442616 - Attachment is patch: true
Assignee

Comment 33

5 years ago
Posted patch Part 5: backout f05fbe6261ac (obsolete) β€” β€” Splinter Review
Assignee

Comment 34

5 years ago
(In reply to Andrew Halberstadt [:ahal] from comment #7)
> Actually that middle changeset looks like it shouldn't be backed out.. so:
> 
> https://hg.mozilla.org/mozilla-central/rev/e1d54d299ab3
> https://hg.mozilla.org/mozilla-central/rev/d4d29af7b3d0
> https://hg.mozilla.org/mozilla-central/rev/b36a923d9b4a
> https://hg.mozilla.org/mozilla-central/rev/f05fbe6261ac
There are two conflicts while backout f05fbe6261ac, had merge back into backout_f05fbe6261ac

Try result:
https://tbpl.mozilla.org/?tree=Try&rev=d7023aa6b9bc
Comment on attachment 8442618 [details] [diff] [review]
Part 5: backout f05fbe6261ac

Review of attachment 8442618 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/reftests/flexbox/reftest.list
@@ +118,2 @@
>  == flexbox-inlinecontent-horiz-3a.xhtml flexbox-inlinecontent-horiz-3-ref.xhtml
> +<<<<<<< local

This patch does not merge correctly. That's why it causes bug 1027501. However bug 1027501 is a potential error, it still needs to be fixed.

::: layout/xul/reftest/reftest.list
@@ +1,1 @@
> +<<<<<<< local

Ditto.
Assignee

Comment 36

5 years ago
Posted patch Part 5: backout f05fbe6261ac (obsolete) β€” β€” Splinter Review
Correct merge conflict
Attachment #8442618 - Attachment is obsolete: true
Assignee

Comment 37

5 years ago
(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #35)
> Comment on attachment 8442618 [details] [diff] [review]
> Part 5: backout f05fbe6261ac
> 
> Review of attachment 8442618 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/reftests/flexbox/reftest.list
> @@ +118,2 @@
> >  == flexbox-inlinecontent-horiz-3a.xhtml flexbox-inlinecontent-horiz-3-ref.xhtml
> > +<<<<<<< local
> 
> This patch does not merge correctly. That's why it causes bug 1027501.
> However bug 1027501 is a potential error, it still needs to be fixed.
> 
> ::: layout/xul/reftest/reftest.list
> @@ +1,1 @@
> > +<<<<<<< local
> 
> Ditto.

Thinks!! 
Fix merge conflict and submit a new try again
https://tbpl.mozilla.org/?tree=Try&rev=07cb963d9951
Assignee

Comment 38

5 years ago
Posted patch Part 5: backout f05fbe6261ac (obsolete) β€” β€” Splinter Review
Attachment #8442691 - Attachment is obsolete: true
Assignee

Comment 39

5 years ago
reftest-remote only try result:
https://tbpl.mozilla.org/?tree=Try&rev=95635d80c450

full reftest try result(base on the latest version)
https://tbpl.mozilla.org/?tree=Try&rev=b360e1cc611c
Assignee

Comment 40

5 years ago
Posted patch Part 2: backout e1d54d299ab3 (obsolete) β€” β€” Splinter Review
Attachment #8442615 - Attachment is obsolete: true
Assignee

Comment 41

5 years ago
Posted patch Part 5: backout f05fbe6261ac (obsolete) β€” β€” Splinter Review
Attachment #8442725 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Attachment #8442750 - Attachment is patch: true
Assignee

Comment 42

5 years ago
Try result of rolling back random-if patches in bug 981477
https://tbpl.mozilla.org/?tree=Try&rev=b360e1cc611c 

1. A skip item which is also fixed by this patch
a. skip-if(B2G&&browserIsRemote) == box-ordinal-with-out-of-flow-1.html ...

2. Need to file a bug to trace status
layout/reftests/bugs/reftest.list
[Unknown] random-if(Android||(B2G&&browserIsRemote)) == 728983-1.html 728983-1-ref.html 
[scrollbar] random-if(B2G&&browserIsRemote) == 960822.html 960822-ref.html..
[scrollbar] random-if(B2G&&browserIsRemote) == 381497-n.html 381497-f.html ..

3. There are associated bugs on XP. But we may still need to file issues for B2G
[Bug 943863 XP] random-if(B2G&&browserIsRemote) == box-sizing-replaced-002.xht ..
[Bug 935850 XP] random-if(B2G&&browserIsRemote) == box-sizing-replaced-003.xht ..
[Bug 629416 XP] random-if(B2G&&browserIsRemote) == 413928-1.html 413928-1-ref.html
[Bug 629416 XP] random-if(B2G&&browserIsRemote) == 413928-2.html 413928-2-ref.html

4. Know issues which still not assign
[Bug 982547] random-if(Android) skip-if(B2G&&browserIsRemote) == box-sizing-replaced-001.xht..
[Bug 988759] random-if(B2G&&browserIsRemote) == multipleinsertionpoints-ref2.xhtml ...
[Bug 988763] random-if(B2G&&browserIsRemote) == referenced-from-binding-01.html ...
[Bug 988765] random-if(B2G&&browserIsRemote) == 166591-dynamic-1.html ...
[Bug 972697] skip-if(B2G&&browserIsRemote) include font-inflation/reftest.list
Assignee

Comment 43

5 years ago
(In reply to Andrew Halberstadt [:ahal] from comment #7)
> Actually that middle changeset looks like it shouldn't be backed out.. so:
> 
> https://hg.mozilla.org/mozilla-central/rev/e1d54d299ab3
> https://hg.mozilla.org/mozilla-central/rev/d4d29af7b3d0
> https://hg.mozilla.org/mozilla-central/rev/b36a923d9b4a
> https://hg.mozilla.org/mozilla-central/rev/f05fbe6261ac

Hi ahal,
Back out these four patches re-enable many failed test cases which are not fixed yet. For example, XUL test case and font-inflation ones.

I am going to roll back these four patches and manually disable failed ones in this bug, may I have your opinion here? Thanks.
Flags: needinfo?(ahalberstadt)
Assignee

Updated

5 years ago
Blocks: B2GRT
Assignee

Comment 44

5 years ago
These four test cases are also pass after this fix

layout/reftests/mathml/reftest.list
  fails-if(B2G&&browserIsRemote) == tablespacing-4.html tablespacing-4-ref.html
  fails-if(B2G&&browserIsRemote) == tablespacing-5.html tablespacing-5-ref.html
  fails-if(B2G&&browserIsRemote) == tablespacing-5a.html tablespacing-5a-ref.html
  fails-if(B2G&&browserIsRemote) == tablespacing-6.html tablespacing-6-ref.html
Assignee

Updated

5 years ago
Duplicate of this bug: 1018381
(In reply to C.J. Ku[:cjku] from comment #43)
> Hi ahal,
> Back out these four patches re-enable many failed test cases which are not
> fixed yet. For example, XUL test case and font-inflation ones.
> 
> I am going to roll back these four patches and manually disable failed ones
> in this bug, may I have your opinion here? Thanks.

Yes I think that's the best way to do it. Though you might as well land part 1 now (assuming it doesn't cause any orange).
Flags: needinfo?(ahalberstadt)
Assignee

Comment 47

5 years ago
(In reply to Andrew Halberstadt [:ahal] from comment #46)
> Yes I think that's the best way to do it. Though you might as well land part
> 1 now (assuming it doesn't cause any orange).
Unfortunately, it does cause some orange.
For example, these four reftest failed
fails-if(B2G&&browserIsRemote) == tablespacing-4.html tablespacing-4-ref.html
fails-if(B2G&&browserIsRemote) == tablespacing-5.html tablespacing-5-ref.html
fails-if(B2G&&browserIsRemote) == tablespacing-5a.html tablespacing-5a-ref.html
fails-if(B2G&&browserIsRemote) == tablespacing-6.html tablespacing-6-ref.html

I need to remove some "fails-if(B2G&&browserIsRemote)" before check in this patch.
Assignee

Comment 48

5 years ago
Posted patch Part 5: backout f05fbe6261ac (obsolete) β€” β€” Splinter Review
Attachment #8442752 - Attachment is obsolete: true
Assignee

Comment 49

5 years ago
Posted patch Part 5: backout f05fbe6261ac (obsolete) β€” β€” Splinter Review
Attachment #8443229 - Attachment is obsolete: true
Assignee

Comment 50

5 years ago
Posted patch Part 5: backout f05fbe6261ac (obsolete) β€” β€” Splinter Review
Attachment #8443240 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Attachment #8443312 - Attachment is patch: true
Assignee

Comment 51

5 years ago
Posted patch Part 5: backout f05fbe6261ac (obsolete) β€” β€” Splinter Review
Disable more xul test case
Attachment #8443312 - Attachment is obsolete: true
Assignee

Comment 52

5 years ago
Posted patch Part 5: backout f05fbe6261ac (obsolete) β€” β€” Splinter Review
Try result:
https://tbpl.mozilla.org/?tree=Try&rev=6122be44353e
Attachment #8443384 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Attachment #8442616 - Flags: review?(ahalberstadt)
Assignee

Updated

5 years ago
Attachment #8442617 - Flags: review?(ahalberstadt)
Assignee

Updated

5 years ago
Attachment #8442750 - Flags: review?(ahalberstadt)
Assignee

Comment 53

5 years ago
Comment on attachment 8443390 [details] [diff] [review]
Part 5: backout f05fbe6261ac

Hi ahal,
Please help to review these patch, thanks.

Except attachment 8443390 [details] [diff] [review], all the other tree are just roll back change in bug 981477.
Attachment #8443390 - Flags: review?(ahalberstadt)
Assignee

Comment 55

5 years ago
Posted patch Part 5: backout f05fbe6261ac (obsolete) β€” β€” Splinter Review
Oops...more xul found :(
Attachment #8443390 - Attachment is obsolete: true
Attachment #8443390 - Flags: review?(ahalberstadt)
Attachment #8443428 - Flags: review?(ahalberstadt)
C.J, could you fold all the backout patches into one? It would make it a lot easier to review.
Flags: needinfo?(cku)
Assignee

Comment 57

5 years ago
Sure, doing, wait a minute :)
Flags: needinfo?(cku)
Assignee

Comment 58

5 years ago
Attachment #8442616 - Attachment is obsolete: true
Attachment #8442617 - Attachment is obsolete: true
Attachment #8442750 - Attachment is obsolete: true
Attachment #8443428 - Attachment is obsolete: true
Attachment #8442616 - Flags: review?(ahalberstadt)
Attachment #8442617 - Flags: review?(ahalberstadt)
Attachment #8442750 - Flags: review?(ahalberstadt)
Attachment #8443428 - Flags: review?(ahalberstadt)
Assignee

Comment 59

5 years ago
remove trail space
Attachment #8443491 - Attachment is obsolete: true
Assignee

Comment 60

5 years ago
Comment on attachment 8443491 [details] [diff] [review]
Part 2: rollback disable test in bug 981477

Combined patch
Attachment #8443491 - Flags: review?(ahalberstadt)
Assignee

Updated

5 years ago
Attachment #8443503 - Flags: review?(ahalberstadt)
Assignee

Comment 61

5 years ago
add email and reviewer in patch
Attachment #8443503 - Attachment is obsolete: true
Attachment #8443503 - Flags: review?(ahalberstadt)
Attachment #8443514 - Flags: review?(ahalberstadt)
Assignee

Updated

5 years ago
Attachment #8441556 - Attachment description: 2 pixels border → Diagram: 2 pixels border
Assignee

Updated

5 years ago
Attachment #8441570 - Attachment description: invalid_area_and_capture_area.png → Diagram: Diff of invalid area and captured area
Comment on attachment 8443514 [details] [diff] [review]
Part 2: rollback disable test in bug 981477

Review of attachment 8443514 [details] [diff] [review]:
-----------------------------------------------------------------

This is great, thanks! I did a last try push here:
https://tbpl.mozilla.org/?tree=Try&rev=ba91e89c106e

We probably shouldn't land this until each job has been re-triggered at least 5 times to see if there are any intermittents still hiding around.

::: layout/reftests/mathml/reftest.list
@@ +101,5 @@
>  == mpadded-1-2.html mpadded-1-2-ref.html
>  == mpadded-6.html mpadded-6-ref.html
> +fails-if(B2G) == mpadded-7.html mpadded-7-ref.html # B2G: slight character width variation
> +fails-if(B2G) == mpadded-8.html mpadded-8-ref.html # B2G: slight character width variation
> +fails-if(B2G) == mpadded-9.html mpadded-9-ref.html # B2G: slight character width variation

I assume these were here before? Otherwise are you sure it fails in the non-oop case too?
Attachment #8443514 - Flags: review?(ahalberstadt) → review+
Assignee

Comment 63

5 years ago
(In reply to Andrew Halberstadt [:ahal] from comment #62)
> ::: layout/reftests/mathml/reftest.list
> @@ +101,5 @@
> >  == mpadded-1-2.html mpadded-1-2-ref.html
> >  == mpadded-6.html mpadded-6-ref.html
> > +fails-if(B2G) == mpadded-7.html mpadded-7-ref.html # B2G: slight character width variation
> > +fails-if(B2G) == mpadded-8.html mpadded-8-ref.html # B2G: slight character width variation
> > +fails-if(B2G) == mpadded-9.html mpadded-9-ref.html # B2G: slight character width variation
> 
> I assume these were here before? Otherwise are you sure it fails in the
> non-oop case too?
Yes, "fails-if(B2G)" is replaced by "random-if(B2G&&browserIsRemote)" at f05fbe6261ac, 
https://hg.mozilla.org/mozilla-central/rev/f05fbe6261ac#l41.57
Assignee

Comment 64

5 years ago
(In reply to Andrew Halberstadt [:ahal] from comment #62)
> This is great, thanks! I did a last try push here:
> https://tbpl.mozilla.org/?tree=Try&rev=ba91e89c106e
ahal, I notice you didn't push attachment 8442612 [details] [diff] [review] in this try round. Without part 1, many test cases will be fail.
Flags: needinfo?(ahalberstadt)
Oh right, good catch! Here's a new one:
https://tbpl.mozilla.org/?tree=Try&rev=7490caf3ad02
Flags: needinfo?(ahalberstadt)
Assignee

Comment 66

5 years ago
(In reply to Andrew Halberstadt [:ahal] from comment #65)
> Oh right, good catch! Here's a new one:
> https://tbpl.mozilla.org/?tree=Try&rev=7490caf3ad02

R9
REFTEST TEST-UNEXPECTED-FAIL
layout/reftests/css-ui-valid/select/reftest.list
-random-if(Android||B2G) == textarea-dyn-disabled.html textarea-ref.html
+random-if(Android) == textarea-dyn-disabled.html textarea-ref.html
Still fail. "Part 1" does not fix this problem

R10
REFTEST TEST-UNEXPECTED-FAIL
layout/reftests/mathml/reftest.list
-random-if(B2G&&browserIsRemote) == multiscripts-1.html multiscripts-1-ref.html # B2G - slight height variation from font metrics
+== multiscripts-1.html multiscripts-1-ref.html # B2G - slight height variation from font metrics
Still fail. I am going to rollback this change.

R10
REFTEST TEST-UNEXPECTED-PASS
fails-if(B2G) == menclose-2-circle.html menclose-2-circle-ref.html
Looks like "Part 1" also fix this bug. I am going to remove fails-if

R17
Bug 906716 - Intermittent B2G emulator "timed out after 1000 seconds of no output"
I never meet this problem in my office time. I don't this failure is relative to applied patches
Assignee

Comment 67

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=540986eaaba3

random-if(Android||B2G) == textarea-dyn-disabled.html textarea-ref.html
random-if(B2G&&browserIsRemote) == multiscripts-1.html multiscripts-1-ref.html
== menclose-2-circle.html menclose-2-circle-ref.html
Assignee

Comment 68

5 years ago
All pass in the 1st try
Trigger the 2nd try
https://tbpl.mozilla.org/?tree=Try&rev=bdc5c7079566
Awesome! For future reference, you can re-trigger a job without doing a new push by clicking the blue '+' in the bottom left (after first clicking a job). I went ahead and re-triggered the jobs on the previous push a few more times.
That looks really green! I think we are good to land.
Assignee

Updated

5 years ago
Attachment #8443881 - Attachment description: Part 2: fix failure in try 7490caf3ad02 → Part 2: rollback disable test in bug 981477
Attachment #8443881 - Flags: review+
Assignee

Updated

5 years ago
Attachment #8443514 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Keywords: checkin-needed
Attachment #8443491 - Flags: review?(ahalberstadt)
Assignee

Comment 73

5 years ago
<svg xmlns="http://www.w3.org/2000/svg" version="1.1" class="reftest-wait"
     style="border: 10px solid lime; padding: 10px; background: lime;">
<rect id="r" width="50" height="50" fill="red"/>
<rect id="f" y="100" width="50" height="50" fill="red"/>
<rect x="100" y="100" width="50" height="50" fill="red"/>
<rect x="200" y="100" width="50" height="50" fill="red"/>
border: 10px + padding: 10px + 200(x pos) + 50(width) = 270

06:37:24     INFO -  REFTEST INFO | Saved log: [CONTENT] Rect: 20 20 270 170
06:37:24     INFO -  REFTEST INFO | Saved log: Updating canvas for invalidation
06:37:24     INFO -  REFTEST INFO | Saved log: DoDrawWindow 20,20,250,150

MozReftestInvalidate fired and invalidate area is correct.

This fail is not relative to Part 1. 
Part 2 remove random-if failure type cause this problem.
Assignee

Comment 74

5 years ago
Hi Ahal,
REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8888/tests/layout/reftests/svg/outer-svg-border-and-padding-01.svg | image comparison (==), max difference: 255, number of differing pixels: 10000
Return code: 1

Before we have attachment 8442612 [details] [diff] [review], this test is 100% fail. After that, this test case turn out to be random failed.
I prefer change failure type back to random-if and file another bug to fix this timing issue, how do you think?
Flags: needinfo?(ahalberstadt)
That sounds good to me. Better to land this before more stuff breaks on us.
Flags: needinfo?(ahalberstadt)
Assignee

Comment 76

5 years ago
Change outer-svg-border-and-padding-01.svg  back to random-if
Attachment #8445284 - Flags: review+
Assignee

Updated

5 years ago
Attachment #8443881 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Attachment #8445284 - Attachment description: Part 2': rollback disable test in bug 981477 → Part 2: rollback disable test in bug 981477
Assignee

Comment 77

5 years ago
attachment 8445284 [details] [diff] [review] 
1. Rollback outer-svg-border-and-padding-01 reftest to random-if
2. remove trailer space of non-scaling-stroke-01.svg test item
Assignee

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/489c599d5c40
https://hg.mozilla.org/mozilla-central/rev/861fbef8e95c
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
This is generally green on Aurora, but it's hitting timeouts in dynamic-use-02.svg ~20% of the time. I'm not sure if it's a consequence of tests shuffling around between chunks or something else, but it's interesting to me that it's not hitting trunk AFAICT.
https://tbpl.mozilla.org/?tree=Try&rev=e5b2f13043af

https://tbpl.mozilla.org/php/getParsedLog.php?id=42440680&tree=Try

Updated

5 years ago
Blocks: 1023906

Updated

5 years ago
Blocks: 1023927
Thanks for getting this fixed and landed.
Assignee

Updated

5 years ago
Duplicate of this bug: 1017493
Assignee

Updated

5 years ago
Duplicate of this bug: 1017517
Assignee

Updated

5 years ago
Duplicate of this bug: 1020017
You need to log in before you can comment on or make changes to this bug.