Remove the mediaEnabled setting

Remove the mediaEnabled setting

This setting only prevented HTMLMediaElement::load() from running, which
very rarely makes any difference, since scripts modifying the DOM will
almost always implicitly invoke resource selection. The exceptions are
adding source elements when networkState is not NETWORK_EMPTY, removing
the src attribute, and removing source elements.

Since the callers to setMediaEnabled(false) seems to have survived
without actually disabling media, just remove the setting for now.

BUG=none

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=167575

[acolwell GONE FROM CHROMIUM, 2014-02-18 19:03:52]

I'm not sure I agree with your premise. Just because the setting wasn't 100%
effective doesn't mean we should remove it. I think we need to understand why
the InspectorOverlay and SVGImage are explicitly trying to disable media
support. If these reasons are still valid then I think we should move the check
to some place like loadTimerFired() so that all loads are effectively blocked.

[philipj_slow, 2014-02-19 03:38:34]

Fair enough :)

The SVGImage case is from this commit:

commit ac06b0abaeb2a22eeb4e4fa0a0d953c3b5ecd16a
Author: japhet@chromium.org
<japhet@chromium.org@bbb929c8-8fbe-4397-9dbb-9b2b20218538>
Date:   Tue Apr 20 21:42:20 2010 +0000

    2010-04-20  Justin Schuh  <jschuh@chromium.org>
    
            Reviewed by Adam Barth.
    
            Invalid cast due to <video> inside <foreignObject> inside <svg>
inside <img>
            https://bugs.webkit.org/show_bug.cgi?id=37331
    
            Added a setting to enable/disable media per-page and have the
SVGImage
            disable media for its dummy page. Also found and fixed a related bad
            cast in the V8 bindings (JSC had a custom wrapper for this already).
    
            Tests: media/svg-as-image-with-media-blocked.html
    
    
    git-svn-id: svn://svn.chromium.org/blink/trunk@57922
bbb929c8-8fbe-4397-9dbb-9b2b20218538

At the time it actually prevented media elements from being created at all,
which is rather different from what it has morphed into. I just noticed that
SVGImage also does setScriptEnabled(false) which makes the setting 0% effective,
as HTMLMediaElement::load() is only called from bindings.

commit 45707d30a6c51e21c5b5813cbf899b9a17fe2ca1
Author: pfeldman@chromium.org
<pfeldman@chromium.org@bbb929c8-8fbe-4397-9dbb-9b2b20218538>
Date:   Tue Aug 28 08:52:26 2012 +0000

    Web Inspector: introduce overlay page and migrate "paused in debugger" to
it.
    https://bugs.webkit.org/show_bug.cgi?id=95080
    
    Reviewed by Yury Semikhatsky.
    
    Separate page instance is now used to render overlay. Paused in debugger is
migrated to the new overlay.
    
    * CMakeLists.txt:
    * DerivedSources.make:
    * GNUmakefile.am:
    * WebCore.gyp/WebCore.gyp:
    * WebCore.gypi:
    * WebCore.vcproj/WebCore.vcproj:
    * WebCore.xcodeproj/project.pbxproj:
    * inspector/InspectorOverlay.cpp:
    (WebCore::InspectorOverlay::~InspectorOverlay):
    (WebCore):
    (WebCore::InspectorOverlay::paint):
    (WebCore::InspectorOverlay::drawOutline):
    (WebCore::InspectorOverlay::setPausedInDebuggerMessage):
    (WebCore::InspectorOverlay::drawNodeHighlight):
    (WebCore::InspectorOverlay::drawRectHighlight):
    (WebCore::InspectorOverlay::drawOverlayPage):
    (WebCore::InspectorOverlay::overlayPage):
    (WebCore::InspectorOverlay::evaluateInOverlay):
    * inspector/InspectorOverlay.h:
    (InspectorOverlay):
    * inspector/InspectorOverlayPage.html: Added.
    * inspector/InspectorPageAgent.cpp:
    (WebCore::InspectorPageAgent::didPaint):
    
    git-svn-id: svn://svn.chromium.org/blink/trunk@126857
bbb929c8-8fbe-4397-9dbb-9b2b20218538

Here it's just one of many settings which are set, so it's not obvious what the
intention is.

I'll email the committers and reviewers to see if they know more.

[acolwell GONE FROM CHROMIUM, 2014-02-19 16:35:04]

Thanks for looking into this. I'm not as worried about the inspector use. The
SVG one I'm more worried about since it looks like the webkit bug may be a
security fix. Does media/svg-as-image-with-media-blocked.html crash if you
remove the setting?

[philipj_slow, 2014-02-19 17:25:30]

On 2014/02/19 16:35:04, acolwell wrote:
> Thanks for looking into this. I'm not as worried about the inspector use. The
> SVG one I'm more worried about since it looks like the webkit bug may be a
> security fix. Does media/svg-as-image-with-media-blocked.html crash if you
> remove the setting?

I doesn't crash with the setting removed. Any observable change at all would be
surprising since that test doesn't involve any scripts that could call load(),
and scripts are supposed to be disabled anyway.

[acolwell GONE FROM CHROMIUM, 2014-02-19 17:32:13]

On 2014/02/19 17:25:30, philipj wrote:
> On 2014/02/19 16:35:04, acolwell wrote:
> > Thanks for looking into this. I'm not as worried about the inspector use.
The
> > SVG one I'm more worried about since it looks like the webkit bug may be a
> > security fix. Does media/svg-as-image-with-media-blocked.html crash if you
> > remove the setting?
> 
> I doesn't crash with the setting removed. Any observable change at all would
be
> surprising since that test doesn't involve any scripts that could call load(),
> and scripts are supposed to be disabled anyway.

Hmm.. ok. lgtm. Thanks for chasing all this down.

[philipj_slow, 2014-02-19 17:41:42]

Thanks! I'll wait 24 hours to give the people I emailed some time to come in and
object before I commit this.

[jschuh, 2014-02-19 21:03:30]

The original CL was to fix a security vulnerability due to a bad cast. It looks
like it evolved quite a bit since then, so my only requirement is that you make
sure you're not reintroducing the vulnerability. However, removing the test
specifically covering the vulnerability this fixed is not exactly confidence
inspiring.

[philipj_slow, 2014-02-20 07:32:58]

On 2014/02/19 21:03:30, Justin Schuh wrote:
> The original CL was to fix a security vulnerability due to a bad cast. It
looks
> like it evolved quite a bit since then, so my only requirement is that you
make
> sure you're not reintroducing the vulnerability. However, removing the test
> specifically covering the vulnerability this fixed is not exactly confidence
> inspiring.

media/svg-as-image-with-media-blocked.html is the test that was added together
with the SVGImage commit referenced above and is left alone.

media/media-disable-crash.html would now fail on
window.internals.settings.setMediaEnabled(false) since the setting is gone, but
would otherwise only do document.createElement("audio"), which is also done in
these tests:

media/W3C/audio/currentSrc/currentSrc_nonempty_after_adding_source_child.html
media/W3C/audio/currentSrc/currentSrc_nonempty_after_setting_src.html
media/invalid-media-url-crash.html
media/media-controls-clone-crash.html
media/media-load-event.html
media/network-no-source-const-shadow.html

[jschuh, 2014-02-21 04:16:29]

On 2014/02/20 07:32:58, philipj wrote:
> On 2014/02/19 21:03:30, Justin Schuh wrote:
> > The original CL was to fix a security vulnerability due to a bad cast. It
> looks
> > like it evolved quite a bit since then, so my only requirement is that you
> make
> > sure you're not reintroducing the vulnerability. However, removing the test
> > specifically covering the vulnerability this fixed is not exactly confidence
> > inspiring.
> 
> media/svg-as-image-with-media-blocked.html is the test that was added together
> with the SVGImage commit referenced above and is left alone.
> 
> media/media-disable-crash.html would now fail on
> window.internals.settings.setMediaEnabled(false) since the setting is gone,
but
> would otherwise only do document.createElement("audio"), which is also done in
> these tests:
> 
> media/W3C/audio/currentSrc/currentSrc_nonempty_after_adding_source_child.html
> media/W3C/audio/currentSrc/currentSrc_nonempty_after_setting_src.html
> media/invalid-media-url-crash.html
> media/media-controls-clone-crash.html
> media/media-load-event.html
> media/network-no-source-const-shadow.html

Gotcha. Then lgtm on my end.

[philipj_slow, 2014-02-21 04:21:54]

The CQ bit was checked by philipj@opera.com

[commit-bot: I haz the power, 2014-02-21 04:22:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/philipj@opera.com/169363004/40001

[commit-bot: I haz the power, 2014-02-21 04:22:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-21 04:22:08]

Failed to apply patch for Source/core/frame/Settings.in:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/frame/Settings.in
  Hunk #1 FAILED at 176.
  1 out of 1 hunk FAILED -- saving rejects to file
Source/core/frame/Settings.in.rej

Patch:       Source/core/frame/Settings.in
Index: Source/core/frame/Settings.in
diff --git a/Source/core/frame/Settings.in b/Source/core/frame/Settings.in
index
5dbb12a52b59ccf7713c0f19d6eee2b2bc9ac0c6..f419b72340930f8af9de0a4b3b0cee7fcbe12043
100644
--- a/Source/core/frame/Settings.in
+++ b/Source/core/frame/Settings.in
@@ -176,7 +176,6 @@ compositorTouchHitTesting initial=true
 fixedPositionCreatesStackingContext initial=false
 syncXHRInDocumentsEnabled initial=true
 cookieEnabled initial=true
-mediaEnabled initial=true
 DOMPasteAllowed initial=false
 
 threadedHTMLParser initial=false

[philipj_slow, 2014-02-21 04:27:23]

The CQ bit was checked by philipj@opera.com

[commit-bot: I haz the power, 2014-02-21 04:27:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/philipj@opera.com/169363004/260001

[commit-bot: I haz the power, 2014-02-21 06:49:55]

Change committed as 167575