Last Comment Bug 716751 - matchMedia() listeners lost
: matchMedia() listeners lost
Product: Core
Classification: Components
Component: DOM: CSS Object Model (show other bugs)
: Trunk
: All All
P2 normal (vote)
: mozilla13
Assigned To: David Baron :dbaron: ⌚️UTC-8
: Jet Villegas (:jet)
Depends on:
  Show dependency treegraph
Reported: 2012-01-09 17:31 PST by Nicholas C. Zakas
Modified: 2012-03-01 06:31 PST (History)
5 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch (11.73 KB, patch)
2012-02-29 11:36 PST, David Baron :dbaron: ⌚️UTC-8
bzbarsky: review-
Details | Diff | Splinter Review

Description User image Nicholas C. Zakas 2012-01-09 17:31:00 PST
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)");
    console.log("HERE: " + window.innerWidth);
Comment 1 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-01-09 19:13:34 PST
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 User image David Baron :dbaron: ⌚️UTC-8 2012-02-29 11:36:31 PST
Created attachment 601686 [details] [diff] [review]
Comment 3 User image Boris Zbarsky [:bz] (still a bit busy) 2012-02-29 19:37:44 PST
Comment on attachment 601686 [details] [diff] [review]

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 User image David Baron :dbaron: ⌚️UTC-8 2012-02-29 20:24:17 PST
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)) {
+    if (!HasListeners()) {
+      // Append failed; undo the AddRef above.
+    }
Comment 5 User image Boris Zbarsky [:bz] (still a bit busy) 2012-02-29 20:25:51 PST
Yes.  r=me with that change.
Comment 6 User image David Baron :dbaron: ⌚️UTC-8 2012-02-29 20:50:34 PST
Comment 7 User image Marco Bonardo [::mak] 2012-03-01 06:31:39 PST

Note You need to log in before you can comment on or make changes to this bug.