Bug 117825 – REGRESSION (r150663): Using webkitAudioContext in Inspector makes it undefined everywhere

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Sindre Aa]

For the last week or more, webkitAudioContext is not set.

Inspector autocompletes the name, but still returns undefined:
> webkitAudioContext
< undefined

> new webkitAudioContext
< TypeError: 'undefined' is not a constructor (evaluating 'new webkitAudioContext')


offlineAudioContext seems to work:
> webkitOfflineAudioContext
< webkitOfflineAudioContextConstructor

I always end up reverting to r150366, in which Web Audio is working. There were quite a few builds after this that created horrible popping/scratching noises when initializing the audio context, and now it is simply not defined.
I can reproduce it on any page. Just open the console on the welcome-screen when opening the fresh install (r151760) and try to create a context. I have tried restarting the browser. Not sure if I have restarted the mac after installing, so I could try that as well.

[Alexey Proskuryakov]

Regressed in <http://trac.webkit.org/changeset/150663>.

What I'm observing is that webkitAudioContext works in a document until I try to access it from Web Inspector, at which time it disappears from the document too.

Unsure if this is limited to webkitAudioContext, or a more widespread problem.

[Alexey Proskuryakov]

<rdar://problem/14217549>

[Chris Dumez]

new webkitAudioContext(); works in WebInspector for WebKit2 EFL. window.webkitAudioContext is not undefined either. I'm looking into the issue but it is likely to be a setting problem if it works on EFL but not mac port. The constructor is enabled at runtime via RuntimeEnabledSettings.

[Chris Dumez]

RuntimeEnabledFeatures::webkitAudioContextEnabled() is called to check if the constructor should be enabled at runtime. It returns true if the WebAudio setting is enabled.

[Chris Dumez]

One difference in behavior after the patch is that WebAudio used to be a per-PageGroup preference. Now, it is a global preference because it is part of RuntimeEnabledFeatures.

It is possible that the WebAudio setting get disabled somehow when the Safari Web Inspector is showed? This would explain why the constructor would disappear from the document when the inspector is used.

[Alexey Proskuryakov]

Yes, Web Inspector does not enable WebAudio (why would it?).

Clients should be free to apply different settings to multiple PageGroups. The idea of PageGroups is that there are entirely separate, with code creating each not necessarily aware that there are others.

[Chris Dumez]

(In reply to comment #6)
> Yes, Web Inspector does not enable WebAudio (why would it?).

So the webkitAudioContext constructor should not be exposed in the Web inspector, right?

> 
> Clients should be free to apply different settings to multiple PageGroups. The idea of PageGroups is that there are entirely separate, with code creating each not necessarily aware that there are others.

I understand, I wasn't sure of the value of having WebAudio support at pagegroup level. But I understand this is confusing in the current state. For the record, I this is not the only setting behaving like this. I followed the model of CSSRegions and CSSCompositing.

I guess I will roll out. We would need support in the JSC bindings generator for something similar to [EnabledAtRuntime] but using Settings instead of RuntimeEnabledFeatures.

[Alexey Proskuryakov]

> So the webkitAudioContext constructor should not be exposed in the Web inspector, right?

Probably not to Web Inspector code (unless there is a specific reason to need it, in which Web Inspector will opt into it).

Should obviously be exposed in Web Inspector console, as that runs in the context of the inspected page.

> I followed the model of CSSRegions and CSSCompositing

So, the same issue exists for these? That's quite unfortunate :(

[Chris Dumez]

Created attachment 205120 [details]
roll out

[EFL EWS Bot]

Comment on attachment 205120 [details]
roll out

Attachment 205120 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/858693

[Chris Dumez]

(In reply to comment #8)
> > So the webkitAudioContext constructor should not be exposed in the Web inspector, right?
> 
> Probably not to Web Inspector code (unless there is a specific reason to need it, in which Web Inspector will opt into it).
> 
> Should obviously be exposed in Web Inspector console, as that runs in the context of the inspected page.
> 
> > I followed the model of CSSRegions and CSSCompositing
> 
> So, the same issue exists for these? That's quite unfortunate :(

They may not have exactly the same issue, it depends if their constructor is actually enabled at runtime. However, their WebKit2 setting is linked to a global boolean in RuntimeEnabledFeatures:
http://trac.webkit.org/browser/trunk/Source/WebKit2/WebProcess/WebPage/WebPage.cpp#L2490
http://trac.webkit.org/browser/trunk/Source/WebKit2/WebProcess/WebPage/WebPage.cpp#L2491

So changing these settings would affect all page groups.

[Build Bot]

Comment on attachment 205120 [details]
roll out

Attachment 205120 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/956138

[EFL EWS Bot]

Comment on attachment 205120 [details]
roll out

Attachment 205120 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/937394

[Chris Dumez]

Ok, rolling it out is not easy but I am working on it :)

[Build Bot]

Comment on attachment 205120 [details]
roll out

Attachment 205120 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/857654

[Chris Dumez]

Created attachment 205126 [details]
Patch

[Chris Dumez]

Created attachment 205127 [details]
JSDOMWindow.cpp diff

[Kentaro Hara]

Comment on attachment 205126 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=205126&action=review

> Source/WebCore/ChangeLog:9
> +        RuntimeEnabledFeatures.

Would it be hard to change WebAudio so that it uses RuntimeEnabledFeatures?

[Kentaro Hara]

Comment on attachment 205126 [details]
Patch

For the time being, I'll r+ it as this CL is a reasonable revert for r150663, which caused the regression. However, we might want to invent a cleaner solution (e.g. WebAudio should use RuntimeEnabledFeatures).

[kov's GTK+ EWS bot]

Comment on attachment 205126 [details]
Patch

Attachment 205126 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/918432

[Sindre Aa]

By the way, the reason I discovered this was not because it was not found through the inspector, but because my project (which uses webAudio) failed to create the webkitAudioContext. It might be because the inspector was open at the same time, in another tab?

Now (still on the same build) it seems to work occasionally (not consistently). Sometimes I need to refresh the page and close other tabs to get it working again.

[Chris Dumez]

Created attachment 205155 [details]
Patch for landing

[Kentaro Hara]

Comment on attachment 205155 [details]
Patch for landing

Looks OK as an immediate fix for the regression, though we might want to invent a cleaner solution.

[Chris Dumez]

(In reply to comment #19)
> (From update of attachment 205126 [details])
> For the time being, I'll r+ it as this CL is a reasonable revert for r150663, which caused the regression. However, we might want to invent a cleaner solution (e.g. WebAudio should use RuntimeEnabledFeatures).

Do we really want to use RuntimeEnabledFeatures to enable features at runtime? What is the issue with using Settings?

I was thinking that we might want to get rid of RuntimeEnabledFeatures class (Which is barely used at the moment) and use Settings instead?

[Kentaro Hara]

(In reply to comment #24)
> Do we really want to use RuntimeEnabledFeatures to enable features at runtime? What is the issue with using Settings?
> 
> I was thinking that we might want to get rid of RuntimeEnabledFeatures class (Which is barely used at the moment) and use Settings instead?

Sorry for not being clear (I was confused a bit). My point is that it is nasty that (1) we have both [EnabledAtRuntime] and [EnabledBySettings] and that (2) RuntimeEnabledFeatures.h is hand-written. So replacing RuntimeEnabledFeatures with Settings sounds like a reasonable approach.

[WebKit Commit Bot]

Comment on attachment 205155 [details]
Patch for landing

Clearing flags on attachment: 205155

Committed r151832: <http://trac.webkit.org/changeset/151832>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Geoffrey Garen]

> I was thinking that we might want to get rid of RuntimeEnabledFeatures class (Which is barely used at the moment) and use Settings instead?

+1