716751 - matchMedia() listeners lost

Status: RESOLVED FIXED

(Core :: DOM: CSS Object Model, defect, P2)

Description

[Nicholas C. Zakas]

User Agent: Mozilla/5.0 (Windows NT 5.1) AppleWebKit/535.11 (KHTML, like Gecko) Chrome/17.0.963.26 Safari/535.11

Steps to reproduce:

window.matchMedia("screen and (max-width:600px)").addListener(function(media){
    console.log("HERE: " + window.innerWidth);
});

Then resized my browser several times.


Actual results:

The event handler fired three times and then stopped. In some cases, it fired zero times.


Expected results:

It should have continued to fire repeatedly. The only way to make this happen was to cache the MediaQueryList object, such as:

var mql = window.matchMedia("screen and (max-width:600px)");
mql.addListener(function(media){
    console.log("HERE: " + window.innerWidth);
});

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yeah, we need to hold an ownin reference from the window/document/presshell to the returned object as long as it has listeners registered. Also, this should have used events, oh well.

Comment 2

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

Attachment: patch

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 601686 [details] [diff] [review]
patch

mListeners is a fallible array, right?  So we could addref but fail to append in AddListener, then end up leaking on the next AddListener call, as far as I can tell.

Comment 4

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

Oh, for some reason I read that as InfallibleTArray.  I'd sort of thought that's what I'd written, too.  But so it is.

The most straightforward fix would seem to be (in AddListener):

   if (!mListeners.Contains(aListener)) {
     mListeners.AppendElement(aListener);
+    if (!HasListeners()) {
+      // Append failed; undo the AddRef above.
+      NS_RELEASE_THIS();
+    }
   }

Comment 5

[Boris Zbarsky [:bzbarsky]]

Yes.  r=me with that change.

Comment 6

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/2a388b0077d5

Comment 7

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/2a388b0077d5