936129 - nsGlobalWindow::InnerForSetTimeoutOrInterval is a no-op for bareword setTimeout

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

Because WebIDL will use the global of the function as the "this", so we'll have an inner window here and not do anything.

We should probably check that if we have an inner it's the current inner or something?

Still figuring out why this didn't immediately regress with bug 918345 and instead apparently regressed with bug 932322.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Ah, here we are.  nsGlobalWindow::SetTimeoutOrInterval has a null-check for mDoc that was causing us to bail early before.  But now we don't null mDoc out.

We should probably look at all the other cases that forward to inner and see whether they need adjustment now that they get run on a possibly-non-current inner....

Comment 2

[Boris Zbarsky [:bzbarsky]]

Amusingly enough, pre bug 918345 we also got called on an inner... but the current inner!  So in fact a setTimeout call in a navigated-away-from page used to set the timeout on the new page.

Comment 3

[alco]

I am trying to figure out the criticality here. Could this affect any major browswer issues or impact many users?
Based on bug 932322 I'd say that browser performance is affected.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Performance is not affected.

Criticality is unclear to me so far, because I can't tell whether this is really a regression over Firefox 25, say; testing this stuff is complicated.  In particular, it's not clear to me whether the goal here is to prevent setTimeout in unloaded windows or to prevent an unloaded window setting a timeout in a different window.  The latter part works fine; it's the former that's broken here.

Comment 5

[Alex Keybl [:akeybl]]

Thanks bz, we'll wait for more info before tracking

Comment 6

[Peter Van der Beken [:peterv]]

Talked to bz, we're going to make setTimeout a noop for non-current inners. Criticality is unclear to me too.

Comment 7

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 8

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8340455 [details] [diff] [review]
v1

>-  FORWARD_TO_INNER(SetTimeoutOrInterval, (aHandler, interval, aIsInterval, aReturn),

Should this be asserting we have an inner instead?

Can we return -1 in the !inner cases to make it clear that nothing happened?

r=me modulo those nits

Comment 10

[Benjamin Kerensa [:bkerensa]]

We are not going to track this for 28 but you can request an uplift later

Comment 11

[Peter Van der Beken [:peterv]]

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

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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