Label runnables under dom/media/webaudio

RESOLVED FIXED in Firefox 55

Status

()

Core
Web Audio
P1
normal
Rank:
19
RESOLVED FIXED
9 months ago
8 months ago

People

(Reporter: jwwang, Assigned: padenot)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [QDL][BACKLOG][MEDIA])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

9 months ago
See https://wiki.mozilla.org/Quantum/DOM for the motivation.
(Reporter)

Updated

9 months ago
Blocks: 1341539
Rank: 19
Priority: -- → P1
(Reporter)

Comment 1

9 months ago
Hi Paul,
Can you find someone to take this bug? Thanks!
Flags: needinfo?(padenot)
(Assignee)

Updated

9 months ago
Assignee: nobody → padenot
Flags: needinfo?(padenot)
Whiteboard: [QDL][BACKLOG][MEDIA]
Comment hidden (mozreview-request)
(Assignee)

Comment 3

8 months ago
Bill, I didn't know who to request this from, so I picked you, feel free to delegate. I'm not too familiar with all this, but what I did seem to work.

One question I have, though, is that sometimes, in Web Audio, we want to dispatch something to the main thread, but at this time, the global has gone away already (it's highly asynchronous, and depends in system API that take some time, and all, and people can close time at any time).

I don't check that the global is valid when we're in a stack frame that is called _synchronously_ from js or from another main thread runnable, my assumption being that the global cannot go away during the execution of the current runnable. Does that sound correct ?

For example, in AudioContext::OnStateChanged, we're here from deep down inside the MSG, so we don't have any guarantee that the document hasn't gone away, so I nullcheck, but in AsyncDecodeWebAudio, we have js code up the stack so I think it's safe.

Also, what I'm not sure about is the change from `NS_DispatchToMainThread(this);`, is that a correct way to rewrite it ?

Comment 4

8 months ago
mozreview-review
Comment on attachment 8854889 [details]
Bug 1341549 - Label runnables in dom/media/webaudio/

https://reviewboard.mozilla.org/r/126838/#review129662

Thanks Paul. Everything in the patch looks good. I just have two comments:
- I think it might be a bit cleaner to add a Dispatch method to AudioContext. That would reduce the amount of boilerplate that we have here for getting the AbstractMainThread and calling Dispatch on it.
- You can use do_AddRef instead of assigning to a RefPtr and then forget()ing it. This won't always fit on one line, so use at your discretion. But it helps sometimes.
Attachment #8854889 - Flags: review?(wmccloskey) → review+
(Assignee)

Comment 5

8 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c78ccd43636caabe56fd8c86748e6e9f3f80b3f

Comment 6

8 months ago
Pushed by paul@paul.cx:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a436c8af1ba2
Label runnables in dom/media/webaudio/ r=billm

Comment 7

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a436c8af1ba2
Status: NEW → RESOLVED
Last Resolved: 8 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.