771043 - MediaQueryList doesn't dispatch events after entering responsive design mode

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Brian Birtles (:birtles)]

Attachment: Inner SVG used in test case

If you enter responsive design mode and don't refresh the page, then MediaQueryList on child content fails to dispatch events.

Steps to reproduce:

1. Open test case attachment
2 [review]. Open Web Console (Ctrl+Shift+K on Windows)
3. Open Responsive Design Mode (Ctrl+Shift+M on Windows)
4. Check console for notifications
5. Press rotate button
6. Check console for notifications
7. Press rotate button again
8. Check console for notifications

Expected results:

At steps (6) and (8) there should be messages indicating a matchMedia query event was received followed by a transition end. There probably shouldn't be one at step (4).

Actual results:

There are messages showing transitionend events were received at steps 4, 6, and 8 but no match media events.

If, after step (4), i.e. whilst in responsive design mode, one refreshes the page, the events are dispatched as expected. However, even after doing that, exiting and re-entering responsive design mode will cause the problem to re-occur.

Comment 1

[Brian Birtles (:birtles)]

Attachment: Inner SVG used in test case

Comment 2

[Brian Birtles (:birtles)]

Attachment: Test case

Comment 3

[Leon Sorokin]

came to re-report this (apparently). is there any planned movement on this? thanks!

Comment 4

[Archil Imnadze]

I've just come across to this bug and came here to report.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Cameron, do you have cycles to look into this?

Comment 6

[Cameron McCormack (:heycam)]

I don't, sorry; I'm busy this week and likely away for October.

Comment 7

[Boris Zbarsky [:bzbarsky]]

OK, I see what's going on here.

The page is grabbing the MediaQueryList up front.  Then we enter responsive design mode and this creates a new prescontext.  Unfortunately, the MediaQueryList hangs off the prescontext, so now nothing inside Gecko has a pointer to that MediaQueryList anymore.

David, is there a reason we hung these off the prescontext, not the document?

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I don't think so; I suspect I wasn't thinking about the distinction at the time and it seemed easier.  It's been a while, though.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Move MediaQueryList tracking from the prescontext to the document, so they will correctly outlive prescontext changes

Comment 10

[Boris Zbarsky [:bzbarsky]]

Two notes:

1)  A lot of this is just moving code from prescontext to document.

2)  If you'd prefer a web-platform-test here, let me know and I'll try to figure out how to do that.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=00f690eeb849

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Move MediaQueryList tracking from the prescontext to the document, so they will correctly outlive prescontext changes

With a fix for the M5 orange

Comment 12

[Boris Zbarsky [:bzbarsky]]

Updated try push: https://tbpl.mozilla.org/?tree=Try&rev=a03d3fb0cd91

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8494232 [details] [diff] [review]
Move MediaQueryList tracking from the prescontext to the document, so they will correctly outlive prescontext changes

nsIDocument.h:

>+   * Support for window.matchMedia()
>+   */
>+  already_AddRefed<mozilla::dom::MediaQueryList>
>+    MatchMedia(const nsAString& aMediaQueryList);
>+  const PRCList* MediaQueryLists() const {
>+    return &mDOMMediaQueryLists;
>+  }

This is probably more readable with a blank line between the two
method declarations (and maybe before the first as well, to match).

>+  // Our live MediaQueryLists
>+  PRCList               mDOMMediaQueryLists;

Ditch the excess whitespace.

MediaQueryList.cpp:

>+  if (!mDocument->GetShell() || !mDocument->GetShell()->GetPresContext()) {
>+    // XXXbz What's the right behavior here?  Spec doesn't say.
>+    return;
>+  }
>+
>+  mMatches = mMediaList->Matches(mDocument->GetShell()->GetPresContext(),
>+                                nullptr);

Maybe put the shell and/or prescontext in a variable to save
re-fetching?

Also, indent the nullptr one more space (if it's still wrapped).

> nsISupports*
> MediaQueryList::GetParentObject() const
> {
>-  if (!mPresContext) {
>+  if (!mDocument) {
>     return nullptr;
>   }
>-  return mPresContext->Document();
>+  return mDocument;
> }

Make the entire function body just "return mDocument;".


r=dbaron with that


Do we care about a window having more than one document in sequence?
Does that happen?  (XSLT?  document.open?)  (Even if it happens,
maybe we don't care.)

Comment 14

[Boris Zbarsky [:bzbarsky]]

> Also, indent the nullptr one more space (if it's still wrapped).

It's not, but I added Emacs modelines to this file and its header to prevent tabs sneaking in like that again.

> Make the entire function body just "return mDocument;".

Oops.  Yes.

Addressed the other comments.

> Does that happen?

Only in one case, I think: replacement of initial about:blank by a newly loaded document that has the same origin.  This keeps the same Window but creates a new Document.

document.open has the reverse behavior: it keeps the same Document but creates a new Window.

I _could_ store these lists on the inner window instead of the document, I guess.  The spec doesn't actually describe what they're attached to; it probably should.

I _think_ XSLT creates both a new Document and a new Window...

Comment 15

[Boris Zbarsky [:bzbarsky]]

I guess per a very literal reading of cssom this should live on the inner window.  Want me to just move it there?

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

That seems like excess work -- what you have now seems worth landing, and that seems not worth doing until the spec is clearer.

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

One other thought, though -- is there something that makes us send notifications if the characteristics of a new pres context are different from the old one?

Comment 18

[Boris Zbarsky [:bzbarsky]]

The testcase suggests yes; I'll figure out what that something is exactly.

Comment 19

[Boris Zbarsky [:bzbarsky]]

> The testcase suggests yes; I'll figure out what that something is exactly.

What's doing it is this stack:

#16 0x00000001076056cd in mozilla::dom::MediaQueryListListener::Call (this=0x126fbdcd0, list=@0x11f1743e0, aRv=@0x7fff5fbf1020, aExceptionHandling=mozilla::dom::CallbackObject::eReportExceptions) at MediaQueryListBinding.h:71
#17 0x00000001075eeeb6 in nsPresContext::MediaFeatureValuesChanged (this=0x126caf000, aShouldRebuild=nsPresContext::eRebuildStyleIfNeeded, aChangeHint=0) at nsPresContext.cpp:1912
#18 0x0000000107519e5f in nsPresContext::FlushPendingMediaFeatureValuesChanged (this=0x126caf000) at nsPresContext.h:271
#19 0x0000000107501692 in PresShell::FlushPendingNotifications (this=0x13245f400, aFlush={mFlushType = Flush_InterruptibleLayout, mFlushAnimations = true}) at nsPresShell.cpp:4201
#20 0x00000001075004cc in PresShell::FlushPendingNotifications (this=0x13245f400, aType=Flush_InterruptibleLayout) at nsPresShell.cpp:4110
#21 0x0000000107501204 in PresShell::HandlePostedReflowCallbacks (this=0x13245f400, aInterruptible=true) at nsPresShell.cpp:4078
#22 0x00000001074f9fc1 in PresShell::DidDoReflow (this=0x13245f400, aInterruptible=true, aWasInterrupted=false) at nsPresShell.cpp:8717
#23 0x00000001074f8afe in PresShell::ResizeReflowIgnoreOverride (this=0x13245f400, aWidth=6000, aHeight=3000) at nsPresShell.cpp:2072
#24 0x00000001074f8e58 in PresShell::ResizeReflow (this=0x13245f400, aWidth=6000, aHeight=3000) at nsPresShell.cpp:2013
#25 0x00000001069e6782 in nsViewManager::DoSetWindowDimensions (this=0x11d8b3800, aWidth=6000, aHeight=3000) at nsViewManager.cpp:192
#26 0x00000001069e68ea in nsViewManager::SetWindowDimensions (this=0x11d8b3800, aWidth=6000, aHeight=3000) at nsViewManager.cpp:212
#27 0x00000001074e2bfb in nsDocumentViewer::SetBounds (this=0x12cbeab40, aBounds=@0x126ca9a38) at nsDocumentViewer.cpp:1954
#28 0x0000000107c024ce in nsDocShell::SetPositionAndSize (this=0x126ca9800, x=0, y=0, cx=200, cy=100, fRepaint=false) at nsDocShell.cpp:5688
#29 0x0000000107c03047 in non-virtual thunk to nsDocShell::SetPositionAndSize(int, int, int, int, bool) () at nsDocShell.cpp:5672
#30 0x0000000106c739be in nsFrameLoader::UpdateBaseWindowPositionAndSize (this=0x11f4f1b00, aIFrame=0x11d140ad8) at nsFrameLoader.cpp:1851
#31 0x0000000106c7383a in nsFrameLoader::UpdatePositionAndSize (this=0x11f4f1b00, aIFrame=0x11d140ad8) at nsFrameLoader.cpp:1825
#32 0x00000001077251bf in nsSubDocumentFrame::ReflowFinished (this=0x11d140ad8) at nsSubDocumentFrame.cpp:802
#33 0x000000010772522c in non-virtual thunk to nsSubDocumentFrame::ReflowFinished() () at nsSubDocumentFrame.cpp:811
#34 0x00000001075011a5 in PresShell::HandlePostedReflowCallbacks (this=0x12f4d8800, aInterruptible=false) at nsPresShell.cpp:4069
#35 0x00000001074f9fc1 in PresShell::DidDoReflow (this=0x12f4d8800, aInterruptible=false, aWasInterrupted=false) at nsPresShell.cpp:8717

Comment 20

[Boris Zbarsky [:bzbarsky]]

At least that covers the geometry end of things.

Comment 21

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cb3fd4a4b7c5

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Backed out for Android mochitest orange.
https://hg.mozilla.org/integration/mozilla-inbound/rev/40aa7a47fced

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=2753671&repo=mozilla-inbound

Comment 23

[Boris Zbarsky [:bzbarsky]]

Hmph.  For some reason on Android media queries generally don't update sync.  See http://jsfiddle.net/zqw9gjxo/4/

Maybe the issue is that the interruptible-layout flush in the stack above doesn't happen there...

I guess I'll try to rework the test to not rely on that and instead assume notifications will be async.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Did that, and https://hg.mozilla.org/integration/mozilla-inbound/rev/fd343a8b5cfe

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fd343a8b5cfe