Bug 221695 - [GPUP] <audio> won't load when URL ends with .php causing some tests to time out
Summary: [GPUP] <audio> won't load when URL ends with .php causing some tests to time out
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-02-10 10:51 PST by Peng Liu
Modified: 2021-02-11 14:06 PST (History)
12 users (show)

See Also:


Attachments
Patch (5.24 KB, patch)
2021-02-11 11:38 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (6.48 KB, patch)
2021-02-11 11:48 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (7.06 KB, patch)
2021-02-11 11:52 PST, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peng Liu 2021-02-10 10:51:31 PST
http/tests/security/webaudio-render-remote-audio-allowed-crossorigin-redirect.html
http/tests/security/webaudio-render-remote-audio-allowed-crossorigin.html
http/tests/security/webaudio-render-remote-audio-blocked-no-crossorigin-redirect.html
http/tests/security/webaudio-render-remote-audio-blocked-no-crossorigin.html
Comment 1 Chris Dumez 2021-02-10 15:57:03 PST
May not be a webaudio issue. The HTMLAudioElement.play() promise is getting rejected with:
NotSupportedError: The operation is not supported
Comment 2 Chris Dumez 2021-02-10 15:59:49 PST
It is hitting this code path in HTMLMediaElement::play():
```
    if (m_error && m_error->code() == MediaError::MEDIA_ERR_SRC_NOT_SUPPORTED) {
        ERROR_LOG(LOGIDENTIFIER, "rejecting promise because of error");
        promise.reject(NotSupportedError, "The operation is not supported.");
        return;
    }
```
Comment 3 Chris Dumez 2021-02-10 16:18:02 PST
In the GPUProcess, in MediaPlayer::loadWithNextMediaEngine(), nextBestMediaEngine(current) returns null so we end up with m_private being null and lient().mediaPlayerResourceNotSupported() getting called.

m_contentType is {"containerType":"text/php","codecs":"codecs","profiles":"profiles"}
Comment 4 Chris Dumez 2021-02-10 16:20:22 PST
When the GPUProcess is disabled, nextBestMediaEngine() also returns null in this case. However, we end up calling nextMediaEngine() as fallback and this does return a non-null engine.

So now the question is, why isn't nextMediaEngine() getting called in the GPU Process when nextBestMediaEngine() returns null. May be due to the value for m_contentMIMETypeWasInferredFromExtension being different?
Comment 5 Chris Dumez 2021-02-10 16:29:27 PST
(In reply to Chris Dumez from comment #4)
> When the GPUProcess is disabled, nextBestMediaEngine() also returns null in
> this case. However, we end up calling nextMediaEngine() as fallback and this
> does return a non-null engine.
> 
> So now the question is, why isn't nextMediaEngine() getting called in the
> GPU Process when nextBestMediaEngine() returns null. May be due to the value
> for m_contentMIMETypeWasInferredFromExtension being different?

m_contentMIMETypeWasInferredFromExtension is indeed true in the WebProcess but false in the GPUProcess, which is why we don't end up calling nextMediaEngine().

When MediaPlayer::load() gets called in the WebProcess, m_contentType.containerType() returns an empty string so we end up setting the m_contentMIMETypeWasInferredFromExtension flag to true. In the GPUProcess though, when MediaPlayer::load() gets called, m_contentType.containerType() returns "text/php" so we don't end up setting the m_contentMIMETypeWasInferredFromExtension flag.

In any case, this is clearly a media issue, not a WebAudio one.
Comment 6 Chris Dumez 2021-02-10 16:36:44 PST
Basically seems to have regressed Eric's fix in Bug 86308.
Comment 7 Chris Dumez 2021-02-10 17:12:19 PST
This makes 2 out of the 4 tests pass:
diff --git a/Source/WebCore/platform/graphics/MediaPlayer.cpp b/Source/WebCore/platform/graphics/MediaPlayer.cpp
index 2bc8070055ca..7ecf2d09b085 100644
--- a/Source/WebCore/platform/graphics/MediaPlayer.cpp
+++ b/Source/WebCore/platform/graphics/MediaPlayer.cpp
@@ -577,7 +577,7 @@ void MediaPlayer::loadWithNextMediaEngine(const MediaPlayerFactory* current)
     if (m_private) {
 #if ENABLE(MEDIA_SOURCE)
         if (m_mediaSource)
-            m_private->load(m_url, m_contentType, m_mediaSource.get());
+            m_private->load(m_url, m_contentMIMETypeWasInferredFromExtension ? ContentType() : m_contentType, m_mediaSource.get());
         else
 #endif
 #if ENABLE(MEDIA_STREAM)
@@ -585,7 +585,7 @@ void MediaPlayer::loadWithNextMediaEngine(const MediaPlayerFactory* current)
             m_private->load(*m_mediaStream);
         else
 #endif
-        m_private->load(m_url, m_contentType, m_keySystem);
+        m_private->load(m_url, m_contentMIMETypeWasInferredFromExtension ? ContentType() : m_contentType, m_keySystem);
     } else {
         m_private = makeUnique<NullMediaPlayerPrivate>(this);
         if (!m_activeEngineIdentifier && installedMediaEngines().size() > 1 && nextBestMediaEngine(m_currentMediaEngine))

The other 2 tests (the "allowed" variants) still fail because it seems we are rendering silence. I am not sure if this means the fix is wrong or if there are 2 separate bugs.
Comment 8 Chris Dumez 2021-02-11 11:38:37 PST
Created attachment 420014 [details]
Patch
Comment 9 Chris Dumez 2021-02-11 11:48:15 PST
Created attachment 420017 [details]
Patch
Comment 10 Chris Dumez 2021-02-11 11:52:36 PST
Created attachment 420019 [details]
Patch
Comment 11 Eric Carlson 2021-02-11 12:48:56 PST
Comment on attachment 420019 [details]
Patch

Nice detective work, thanks!

r=me once the bots are happy
Comment 12 EWS 2021-02-11 14:05:36 PST
Committed r272750: <https://commits.webkit.org/r272750>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 420019 [details].
Comment 13 Radar WebKit Bug Importer 2021-02-11 14:06:15 PST
<rdar://problem/74250095>