Closed Bug 1295466 Opened 9 years ago Closed 8 years ago

Intermittent w3c-css/submitted/ui3/box-sizing-replaced-001.xht == w3c-css/submitted/ui3/box-sizing-replaced-001-ref.xht | (LOAD ONLY), max difference: 255, number of differing pixels: 1575

Categories

(Core :: Layout, defect, P3)

Product:
Core
Component:
Layout
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox51 --- fixed
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: intermittent-bug-filer, Assigned: neerja)

References

(Depends on 1 open bug)

Details

(Keywords: intermittent-failure)

Attachments

(2 files)

Bug 1295466 - Copy test box-sizing-replaced-001.xht and use non-standard MozReftestInvalidate event to avoid test failure when paint delay is reduced.
58 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1295466 - Revert w3c-css/submitted/ui3/box-sizing-replaced-001.xht to remove previously added MozReftestInvalidate and add fuzzy anotation for this test
58 bytes, text/x-review-board-request
dholbert
: review+
Details
Bulk assigning P3 to all open intermittent bugs without a priority set in Firefox components per bug 1298978.
Priority: -- → P3
Assignee: nobody → npancholi
Depends on: 1283302
No longer depends on: 1283302
Blocks: 1283302
Comment on attachment 8805711 [details] Bug 1295466 - Copy test box-sizing-replaced-001.xht and use non-standard MozReftestInvalidate event to avoid test failure when paint delay is reduced. https://reviewboard.mozilla.org/r/89420/#review89002 Looks like this test needs to be debugged. Marking a reftest as fuzzy with a max difference of 255 is roughly equivalent with disabling it completely.
Attachment #8805711 - Flags: review?(mstange) → review-
Assignee: npancholi → administration
The failing piece of the reftest is #img5, which is sized with the following styles: min-width: 85px; max-height: 85px; padding: 5px 5px; box-sizing: border-box; ...and the element is <img src="[image which is 25px wide and 50px tall]"> When it fails, the right ~1/3 of the element simply isn't painted. I'm guessing that this is some sort of decoding/invalidation dependency bug which is triggered by the reftest snapshot's force-finish-decoding signal. I would bet MozReftestInvalidate will "fix" this, as it did in bug 1178202. We should probably file a followup bug on investigating the underlying issue here -- but that investigation doesn't have to block bug 1283302, as long as we can keep this testcase reliable via MozReftestInvalidate or similar.
Keywords: leave-open
Comment on attachment 8805711 [details] Bug 1295466 - Copy test box-sizing-replaced-001.xht and use non-standard MozReftestInvalidate event to avoid test failure when paint delay is reduced. https://reviewboard.mozilla.org/r/89420/#review91644
Attachment #8805711 - Flags: review?(mstange) → review+
Pushed by mstange@themasta.com: https://hg.mozilla.org/integration/autoland/rev/358036b9cda8 Use MozReftestInvalidate event in box-sizing-replaced-001.xht to avoid test failure when paint delay is reduced r=mstange
No longer blocks: 1283302
So we really shouldn't be using MozReftestInvalidate in tests that are being contributed to the W3C reftests, since it's not going to work for any other browsers. It also seems like this shouldn't really *need* MozReftestInvalidate. Loading of images should work without it. Is there a bug on finding the underlying problem and reverting this change?
Flags: needinfo?(npancholi)
Flags: needinfo?(dholbert)
Oh, hmm... That's a good point. I'd forgotten that this bug was being submitted to the W3C. If this is a bad state to leave this test in, we could also: (1) mark this test as extremely-fuzzy (the bit that was r-minused in comment 15). This kinda disables the test, as mstange noted in his review feedback, but it's a bit better than just marking the test as random. (2) Add a copy of this test *with the MozReftestInvalidate* tweak somewhere else (in a non-w3c-submitted directory), which wouldn't be marked as fuzzy. Then we'd get the benefit of still testing as thoroughly as we're testing now, and being able to ship the reduced paint suppression, all without throwing MozXYZ cruft into the w3c testsuite. > Is there a bug on finding the underlying problem and reverting this change? Not yet, that I'm aware of -- neerja, could you file one, if you haven't already?
Flags: needinfo?(dholbert)
(In reply to Daniel Holbert [:dholbert] from comment #23) > Oh, hmm... That's a good point. I'd forgotten that this bug was being > submitted to the W3C. er, meant to say "I'd forgotten that this *test* was being submitted the w3c test repo"
Depends on: 1316772
Flags: needinfo?(npancholi)
I think we should land an additional patch here that does what I suggested in comment 23. The current test (and all of its image resources) need to be copied somewhere else for the time being -- perhaps into layout/reftests/bugs/1295466-1.html, layout/reftests/bugs/replaced-min-max-1.png, etc. -- and then we should roll back the version in w3c-css/submitted to its previous version, and land the extremely-fuzzy-annotation for that version that mstange previously r-minused here. (Alternatively: we could just go ahead and debug/fix bug 1316772, and roll back the changes to this test without needing to copy it or add any fuzzy annotation. But I'm not sure how much work that'll be, in part because the failure is intermittent. And I don't think that bug is worrisome enough that we should let it hold up bug 1283302's paint-delay improvements. Hence, I'm proposing an interim solution that keeps the "official" version of the test in its official form, and papers over the failure there, and yet still doesn't actually reduce our test coverage.) If anyone has any better ideas, feel free to throw them out!
Flags: needinfo?(npancholi)
(In reply to Daniel Holbert [:dholbert] from comment #25) > I think we should land an additional patch here that does what I suggested > in comment 23. The current test (and all of its image resources) need to be > copied somewhere else for the time being -- perhaps into > layout/reftests/bugs/1295466-1.html, > layout/reftests/bugs/replaced-min-max-1.png, etc. -- and then we should roll > back the version in w3c-css/submitted to its previous version, and land the > extremely-fuzzy-annotation for that version that mstange previously > r-minused here. > > (Alternatively: we could just go ahead and debug/fix bug 1316772, and roll > back the changes to this test without needing to copy it or add any fuzzy > annotation. But I'm not sure how much work that'll be, in part because the > failure is intermittent. And I don't think that bug is worrisome enough > that we should let it hold up bug 1283302's paint-delay improvements. > Hence, I'm proposing an interim solution that keeps the "official" version > of the test in its official form, and papers over the failure there, and yet > still doesn't actually reduce our test coverage.) > > If anyone has any better ideas, feel free to throw them out! I've added two patches that do what dholbert suggested in comment 23. Thanks! (clearing needinfo)
Comment on attachment 8805711 [details] Bug 1295466 - Copy test box-sizing-replaced-001.xht and use non-standard MozReftestInvalidate event to avoid test failure when paint delay is reduced. https://reviewboard.mozilla.org/r/89420/#review92506 ::: layout/reftests/bugs/reftest.list:1977 (Diff revision 5) > # different, but they should use the same button style and the background color > # should be same. |fuzzy()| here allows the difference in border, but not > # background color. > fuzzy(255,1000) skip-if(!cocoaWidget) == 1294102-1.html 1294102-1-ref.html > == 1315632-1.html 1315632-1-ref.html > +fuzzy-if(Android,27,874) fuzzy-if(gtkWidget,14,29) == box-sizing-replaced-002.xht box-sizing-replaced-002-ref.xht # Bug 1128229, Bug 1313772 Please rename these files (using "hg mv") to 1295466-1.xhtml and 1295466-1-ref.xhtml, to match the naming scheme in layout/bugs (which are just corresponding to "the bug number for the bug that prompted us to add this test"), and also to make it easier to distinguish between the two versions of this test. (It'd otherwise be easy to accidentally look up the wrong version of the test, when doing e.g. a DXR search by filename.) And then once you've renamed them, you'll want to insert into reftest.list a bit earlier, to keep this file in numerical order. ::: layout/reftests/bugs/box-sizing-replaced-001.xht:135 (Diff revision 5) > min-height: 85px; > } > </style> > </head> > <script> > - /* this test shouldn't need reftest-wait, but if the reftest snapshot is triggered before we've painted, > + /* this test is a copy of the original w3c-submitted test at w3c-css/submitted/ui3/box-sizing-replaced-001.xht While you're rewriting this comment, please rewrap it to 80 characters wide. (This first line is 113 characters long, which makes it hard to read in a default 80-character-wide terminal.) ::: layout/reftests/bugs/box-sizing-replaced-001.xht:136 (Diff revision 5) > } > </style> > </head> > <script> > - /* this test shouldn't need reftest-wait, but if the reftest snapshot is triggered before we've painted, > - * for img5 the right 1/3rd of the element simply isn't painted. > + /* this test is a copy of the original w3c-submitted test at w3c-css/submitted/ui3/box-sizing-replaced-001.xht > + * we need this copy so that we can use MozReftestInvalidate here without hindering the w3c testsuite * Please capitalize "We" here (It's the beginning of a new sentence, but that's not clear since the previous sentence doesn't end with a period.) * s/MozReftestInvalidate/non-standard MozReftestInvalidate/ (to make it clearer what the problem is with introducing that into the other copy) * Please add a period at the end of this sentence (after "testsuite") so that it doesn't run into the next sentence. ::: layout/reftests/bugs/box-sizing-replaced-001.xht:137 (Diff revision 5) > </style> > </head> > <script> > - /* this test shouldn't need reftest-wait, but if the reftest snapshot is triggered before we've painted, > - * for img5 the right 1/3rd of the element simply isn't painted. > - * See Bug 1283302 > + /* this test is a copy of the original w3c-submitted test at w3c-css/submitted/ui3/box-sizing-replaced-001.xht > + * we need this copy so that we can use MozReftestInvalidate here without hindering the w3c testsuite > + * without MozReftestInvalidate, for img5 the right 1/3rd of the element simply isn't painted. Please capitalize "Without" here, so that it's clearer that it's a new sentence. (I tripped over this the first time I read it and took a few minutes to understand what it's saying, since there's no period or capitalization separating this from the previous sentence. :)) ::: layout/reftests/bugs/box-sizing-replaced-001.xht:139 (Diff revision 5) > <script> > - /* this test shouldn't need reftest-wait, but if the reftest snapshot is triggered before we've painted, > - * for img5 the right 1/3rd of the element simply isn't painted. > - * See Bug 1283302 > + /* this test is a copy of the original w3c-submitted test at w3c-css/submitted/ui3/box-sizing-replaced-001.xht > + * we need this copy so that we can use MozReftestInvalidate here without hindering the w3c testsuite > + * without MozReftestInvalidate, for img5 the right 1/3rd of the element simply isn't painted. > + * this copy can be removed (along with the fuzzy annotation in the original test) when we've fixed the > + * underlying issue at -> Bug 1316772 Let's drop the "->" here. It looks a bit too much like "-->" which is the HTML end-of-comment token (for HTML <!-- ... --> comments), so it feels a bit out of place inside of a JS comment. :)
Attachment #8805711 - Flags: review?(dholbert) → review-
Comment on attachment 8809960 [details] Bug 1295466 - Revert w3c-css/submitted/ui3/box-sizing-replaced-001.xht to remove previously added MozReftestInvalidate and add fuzzy anotation for this test https://reviewboard.mozilla.org/r/92432/#review92512
Attachment #8809960 - Flags: review?(dholbert) → review+
Comment on attachment 8805711 [details] Bug 1295466 - Copy test box-sizing-replaced-001.xht and use non-standard MozReftestInvalidate event to avoid test failure when paint delay is reduced. https://reviewboard.mozilla.org/r/89420/#review92768 Looks good - r=me.
Attachment #8805711 - Flags: review?(dholbert) → review+
Let's get a sanity-check Try reftest run here, to be sure the new copy of the reftest runs successfully. And then we can autoland these followups. Thanks!
(In reply to Daniel Holbert [:dholbert] from comment #37) > Let's get a sanity-check Try reftest run here, to be sure the new copy of > the reftest runs successfully. And then we can autoland these followups. > Thanks! Checked, the copied test passes on my machine. Thanks!
Flags: needinfo?(npancholi)
Comment on attachment 8805711 [details] Bug 1295466 - Copy test box-sizing-replaced-001.xht and use non-standard MozReftestInvalidate event to avoid test failure when paint delay is reduced. https://reviewboard.mozilla.org/r/89420/#review92832
Attachment #8805711 - Flags: review+
(mstange was kind enough to grant explicit r+ on the first followup patch, since MozReview was incorrectly insisting he'd already r+'d that part, with no way around that. I reported this in #mozreview -- I think it stems from the fact that we've had two "waves" of patches landed via MozReview here, which in retrospect is probably not a supported workflow. We probably should have spun off a helper bug for the followup work here. *shrug*. Anyway -- patches landed that achieve comment 23 -- thanks, Neerja!)
(In reply to Daniel Holbert [:dholbert] from comment #40) > have spun off a helper bug for the followup work here. *shrug*. Anyway -- > patches landed that achieve comment 23 -- thanks, Neerja!) Where did these patches land? The tree still looks like it's in the old state.
Flags: needinfo?(npancholi)
Flags: needinfo?(dholbert)
I autolanded them here around when I posted comment 40, I think -- though now I see on MozReview: > There was an error executing the Autoland request on autoland > Requested by dholbert We're sorry, Autoland could not rebase your commits for you automatically. > Please manually rebase your commits and try again. So, you're right -- these patches never landed. Neerja, could you rebase the patches here, and I'll get them autolanded?
(In reply to Daniel Holbert [:dholbert] from comment #42) > I autolanded them here around when I posted comment 40, I think -- though > now I see on MozReview: > > There was an error executing the Autoland request on autoland > > Requested by dholbert We're sorry, Autoland could not rebase your commits for you automatically. > > Please manually rebase your commits and try again. > > So, you're right -- these patches never landed. Neerja, could you rebase the > patches here, and I'll get them autolanded? Done :)
Flags: needinfo?(npancholi)
Pushed by dholbert@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/faa9981549a6 Copy test box-sizing-replaced-001.xht and use non-standard MozReftestInvalidate event to avoid test failure when paint delay is reduced. r=dholbert,mstange https://hg.mozilla.org/integration/autoland/rev/c46aadcf745c Revert w3c-css/submitted/ui3/box-sizing-replaced-001.xht to remove previously added MozReftestInvalidate and add fuzzy anotation for this test r=dholbert
Autolanded now. Thanks for catching that, dbaron! (I think it's appropriate that this will be automatically closed when these patches are merged, since the intermittent failure won't be reported anymore. Bug 1316772 [still open] tracks the underlying issue that we're currently suppressing via fuzzy & MozReftestInvalidate hackarounds.)
Flags: needinfo?(dholbert)
https://hg.mozilla.org/mozilla-central/rev/faa9981549a6 https://hg.mozilla.org/mozilla-central/rev/c46aadcf745c These patches landed yesterday but I think they didn't get marked as such due to the 'Revert' on the message. And the bug is set as leave-open, so I won't close it.
Thanks for the heads-up. "leave-open" state here is stale, and this bug can now be closed as FIXED (with the followup Bug 1316772 tracking the remaining to-do items), per comment 47.
Status: NEW → RESOLVED
Closed: 8 years ago
Keywords: leave-open
Resolution: --- → FIXED
Assignee: administration → npancholi
status-firefox51: --- → affected
status-firefox52: --- → affected
status-firefox53: --- → fixed
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: