Closed Bug 1437680 Opened 6 years ago Closed 6 years ago

Avoid assertions in dom/tests/mochitest/general/test_contentViewer_overrideDPPX.html with the patch for bug 1193394

Categories

(Core :: DOM: Core & HTML, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

(Depends on 1 open bug)

Details

Attachments

(1 file)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=701a987d4fcddd24a4f67f08df6f9ee2b9b81f3a&selectedJob=161715873

GECKO(2914) | Assertion failure: !presContext->HasPendingMediaQueryUpdates() (Someone forgot to update media queries?), at /builds/worker/workspace/build/src/layout/base/ServoRestyleManager.cpp:110

I thought initially it happened for the same reason as bug 1436625, but it seems not.
Flags: needinfo?(hikezoe)
This is pretty unfortunate.  The test case changes dppx inside the callback for MediaQueryListListener, it's processed in DoFlushPendingNotifications() [1].

The right thing we should do is process the callbacks for MediaQueryListListener between dispatching scroll events and animation events (bug 1437688).

That's said, we might want to do a workaround here.

[1] https://hg.mozilla.org/mozilla-central/file/6d8f470b2579/layout/base/PresShell.cpp#l4173
Depends on: 1437688
Flags: needinfo?(hikezoe)
I'd suggest to do workaround here in this bug.  The test does not need to change media feature inside the callbacks for media query list events, so we can add setTimeout() there.  (I am pretty sure the assertion for pending media feature change is the right thing to do)

Here is a try with the setTimeout call.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=538331f2e7bb20bf22f2020fa7743965533f7b64
Summary: Fix assertions in dom/tests/mochitest/general/test_contentViewer_overrideDPPX.html with the patch for bug 1193394 → Avoid assertions in dom/tests/mochitest/general/test_contentViewer_overrideDPPX.html with the patch for bug 1193394
And a try based on the patch for bug 1193394.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f5aa2c7919e463d0dcdcaaea36cf16d1e39c407
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Comment on attachment 8950425 [details]
Bug 1437680 - Don't change media feature in the callback for MediaQueryListEvent.

https://reviewboard.mozilla.org/r/219662/#review225424

Thanks!  Seems okay overall, but please be sure to add a comment.

::: dom/tests/mochitest/general/test_contentViewer_overrideDPPX.html:86
(Diff revision 1)
> +const waitForMediaQueryListEvent = (mediaQueryList) => {
> +  return new Promise(resolve => {
> +    mediaQueryList.addListener(function listener() {
> +      ok(true, "MediaQueryList's listener invoked for " + mediaQueryList.media);
> +      mediaQueryList.removeListener(listener);
> +      setTimeout(resolve, 0);

Please add a comment here explaining why this is needed for future readers.

::: dom/tests/mochitest/general/test_contentViewer_overrideDPPX.html:262
(Diff revision 1)
>      setOverrideDPPX(dppx);
>    },
>    "test OverrideDPPX with MediaQueryList and fullZoom": (done) => {
>      assertValuesAreInitial();
>  
>      let overridden = false;

This line seems unused now, please remove it if so.
Attachment #8950425 - Flags: review?(jryans) → review+
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #5)
> Comment on attachment 8950425 [details]
> Bug 1437680 - Don't change media feature in the callback for
> MediaQueryListEvent.
> 
> https://reviewboard.mozilla.org/r/219662/#review225424
> 
> Thanks!  Seems okay overall, but please be sure to add a comment.
> 
> ::: dom/tests/mochitest/general/test_contentViewer_overrideDPPX.html:86
> (Diff revision 1)
> > +const waitForMediaQueryListEvent = (mediaQueryList) => {
> > +  return new Promise(resolve => {
> > +    mediaQueryList.addListener(function listener() {
> > +      ok(true, "MediaQueryList's listener invoked for " + mediaQueryList.media);
> > +      mediaQueryList.removeListener(listener);
> > +      setTimeout(resolve, 0);
> 
> Please add a comment here explaining why this is needed for future readers.

Indeed.  I added a comment there locally.

> ::: dom/tests/mochitest/general/test_contentViewer_overrideDPPX.html:262
> (Diff revision 1)
> >      setOverrideDPPX(dppx);
> >    },
> >    "test OverrideDPPX with MediaQueryList and fullZoom": (done) => {
> >      assertValuesAreInitial();
> >  
> >      let overridden = false;
> 
> This line seems unused now, please remove it if so.

Good catch!

Thanks for the review!
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/04a85bd02d1e
Don't change media feature in the callback for MediaQueryListEvent. r=jryans
https://hg.mozilla.org/mozilla-central/rev/04a85bd02d1e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: