1339324 - Test that timeouts are indeed throttled in background tabs with paused audio.

Status: NEW

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

Description

[John Lin [:jhlin][:jolin]]

Follow-up of bug 1334825 comment 12.

Comment 1

[John Lin [:jhlin][:jolin]]

Attachment: wip.patch

Comment 2

[John Lin [:jhlin][:jolin]]

Ehsan, when trying to add the 'pause audio' part to your test, I met a problem: calling functions defined in the helper files (file_pluginAudio.html & file_webaudioLoop.html) won't work. ContentTask.spawn will throw exception saying 'content.window.stopAudio' and 'content.window.suspendAC' are not functions ('undefined' I suppose). On the other hand, calling 'setTimeout' seems fine.
Do you by any chance know what's going on there? Is it a limitation of frame script accessing page script objects? Thanks a lot.

wip patch attached in attachment 8837489 [details] [diff] [review].

Comment 3

[(no longer active)]

Hmm, this is strange.  You are passing the correct browser object.  I bet this has something to do with the tab being in the background.  Perhaps Mike can help...

Mike, do you know what the content variable points to inside a frame script?  Is there a special magic that we need if the target of a ContentTask.spawn() is in a background tab?

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Looking at this today - sorry for the delay.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

There are two issues:

1) This one was probably a typo - line 5 of dom/base/test/browser_timeout_throttling_with_audio_playback.js has a syntax error - there's a = between "stopAudio:" and "() => {..." that's causing the test to not parse correctly.

2) By default, I think we try to make it hard for chrome-privileged JS to run functions that have been defined by content - even test content. In this case, "stopAudio" is a function that content has defined. When we look at the content object (which is the same thing as content.window, btw - content === content.window), we're getting the "XRay" view of the underlying window global, without all of the extra stuff that web content has added. This is for security reasons mainly[1], and is something this test is accidentally bumping in to.

The simplest way to proceed here is probably to "waive the XRays" and access the window global in its altered state. I'd normally never recommend that for production code, but this is for a test, so it's probably okay.

You can do that like this:

{ url: "http://mochi.test:8888/browser/dom/base/test/file_pluginAudio.html",
  stopAudio: () => {
    content.wrappedJSObject.stopAudio();
  }
},

Alternatively:


{ url: "http://mochi.test:8888/browser/dom/base/test/file_pluginAudio.html",
  stopAudio: () => {
    let alteredContent = Components.utils.waiveXrays(content);
    alteredContent.stopAudio();
  }
},

Although I don't see that second approach adding much.

Anyhow, if you access through either of these ways, you should be able to call the functions exposed by those .html test pages.

[1]: If you're interested in solidifying your mental model of this security membrane, or even just the specifics and history of it, I highly recommend bholley's talk, Enter the Compartment: https://air.mozilla.org/enter-the-compartment/

Comment 6

[John Lin [:jhlin][:jolin]]

Attachment: Bug 1339324 - add test cases for paused audio.

Review commit: https://reviewboard.mozilla.org/r/114866/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/114866/

Comment 7

[(no longer active)]

Comment on attachment 8840362 [details]
Bug 1339324 - add test cases for paused audio.

https://reviewboard.mozilla.org/r/114866/#review116406

::: dom/base/test/browser_timeout_throttling_with_audio_playback.js:17
(Diff revision 1)
> -// So we use a 10ms minimum timeout value for foreground tabs and a 100,000 second
> +// So we use a 10ms minimum timeout value for foreground tabs and a 3 second
>  // minimum timeout value for background tabs.  This means that in case the test
>  // fails, it will time out in practice, but just for sanity the test condition
>  // ensures that the observed timeout delay falls in this range.
>  const kMinTimeoutForeground = 10;
> -const kMinTimeoutBackground = 100 * 1000 * 1000;
> +const kMinTimeoutBackground = 3 * 1000;

Why did you make this change?  It's entirely possible for a timeout of 3 seconds to be hit without timing out the test.

::: dom/base/test/browser_timeout_throttling_with_audio_playback.js:62
(Diff revision 1)
> +  ContentTask.spawn(newBrowser, {}, () => {
> +    content.wrappedJSObject.stopAudio();
> +  });
> +  yield waitForAudioStopped(newTab);
> +  timeout = yield ContentTask.spawn(newBrowser, {}, asSoonAsPossible);
> +  ok(timeout >= kMinTimeoutBackground, `Got the correct timeout (${timeout})`);

This slows down the test needlessly, in addition to the issue above.

I think a better approach for this case would be to set a timeout of kMinTimeoutForeground+1 in the foreground window, and ensure that by the time that it fires, the timeout in the background window isn't fired.  Then you can use clearTimeout() to clear the timeout in the background window and move on to the next iteration.

::: dom/base/test/file_webaudioLoop.html:4
(Diff revision 1)
>  <!DOCTYPE html>
>  <script>
>  var ac = new AudioContext();
> +var src = undefined;

Just saying `var src;` does the same.

Comment 8

[John Lin [:jhlin][:jolin]]

Comment on attachment 8840362 [details]
Bug 1339324 - add test cases for paused audio.

https://reviewboard.mozilla.org/r/114866/#review116406

> Why did you make this change?  It's entirely possible for a timeout of 3 seconds to be hit without timing out the test.

You're right. This is unnecessary if your approch in next comment is used. Will fix this in next patch.

> This slows down the test needlessly, in addition to the issue above.
> 
> I think a better approach for this case would be to set a timeout of kMinTimeoutForeground+1 in the foreground window, and ensure that by the time that it fires, the timeout in the background window isn't fired.  Then you can use clearTimeout() to clear the timeout in the background window and move on to the next iteration.

This is much better than what I did. Thanks a lot!

> Just saying `var src;` does the same.

Will do.

Comment 9

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Hi John, are you planning to get back to this soon?

Comment 10

[John Lin [:jhlin][:jolin]]

Thanks a lot for the reminder! I totally forgot about it. :$

Anyway, I don't have any plan working on this anytime soon and you're welcome to take it. :)