652317 - Investigate if calling MediaQueryListListener should push Cx to stack

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P2)

Description

[Olli Pettay [:smaug][[e-mail]]]

Traditionally we have pushed JS Context to stack when calling 
asynchronous-like callbacks (event listeners, timeout/interval callbacks, etc.)
Still not long ago that was required for security, but IIRC gal mentioned
that it might not be needed anymore.
But better make sure that is the case.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

... the newly added code
http://hg.mozilla.org/mozilla-central/annotate/de123d952abd/layout/base/nsPresContext.cpp#l1697
does not push context.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Bug 393762 and Bug 383424 are the examples of problems what happened when
context wasn't pushed to stack.

Comment 3

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

Sounds like we should just do it to match other code, and then if we clean stuff up it'll get removed.

Comment 4

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

Attachment: patch

Pushing the inner window seems like the best option to me, given that these listeners were added via an API on window... does it seem right to you?

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

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

>-    for (PRUint32 i = 0, i_end = notifyList.Length(); i != i_end; ++i) {
>-      nsDOMMediaQueryList::HandleChangeData &d = notifyList[i];
>-      d.listener->HandleChange(d.mql);
>+    if (!notifyList.IsEmpty()) {
>+      nsPIDOMWindow *win = mDocument->GetInnerWindow();
>+      nsCOMPtr<nsPIDOMEventTarget> et = do_QueryInterface(win);
>+      nsCxPusher pusher;
>+
>+      if (pusher.Push(et)) {
You need to actually push for each listener separately.
To optimize that, use RePush.


>+        for (PRUint32 i = 0, i_end = notifyList.Length(); i != i_end; ++i) {
>+          nsDOMMediaQueryList::HandleChangeData &d = notifyList[i];
>+          d.listener->HandleChange(d.mql);
>+        }
>+      } else {
>+        NS_NOTREACHED("could not push context");
It is "ok" if you can't push a context. Especially when you do
RePush for each listener. Some listener may actually kill the whole window and
the following listeners can't run in that case.

Comment 6

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

Attachment: patch

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

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

Review of attachment 527957 [details] [diff] [review]:

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/26ba14209541

Comment 9

[Andreas Gal :gal]

Until we have completed the work to remove the context stack its not optional to push onto it.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Hmm.  Does this mean we need to do the same thing for mozRequestAnimationFrame callbacks?

Comment 11

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

That's filed as bug 652401.

I *was* going to unhide this bug today since it was a bug that was only present in one nightly... but now I'm inclined to leave this hidden until bug 652401 is fixed on branches.