Bug 149580 – REGRESSION(r190262): User media unit test failures after r190262

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Carlos Garcia Campos]

The GTK+ unit test /webkit2/WebKitWebView/usermedia-permission-requests started to fail after r190262:

  /webkit2/WebKitWebView/usermedia-permission-requests:                **

ERROR:../../Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestUIClient.cpp:202:static gboolean UIClientTest::permissionRequested(WebKitWebView*, WebKitPermissionRequest*, UIClientTest*): assertion failed: (webkit_user_media_permission_is_for_audio_device(userMediaRequest) == test->m_expectedAudioMedia)

FAIL

But also the cross platform C API test WebKit2.UserMediaBasic is crashing:

**CRASH** WebKit2.UserMediaBasic

[Carlos Garcia Campos]

It seems some layout tests are also affected:

01:50:49.322 5644 worker/1 fast/mediastream/MediaStreamTrackEvent-constructor.html crashed, (stderr lines):
01:50:49.322 5644   1   0x7fda8ab83687 /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18(WTFCrash+0x17) [0x7fda8ab83687]
01:50:49.322 5644   2   0x7fda365792f5 /home/slave/webkitgtk/gtk-linux-64-release-tests/build/WebKitBuild/Release/lib/libTestRunnerInjectedBundle.so(+0xd62f5) [0x7fda365792f5]
01:50:49.322 5644   3   0x7fda8cd68eb7 /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37(_ZN7WebCore10TimerEvent10timerFiredEv+0x17) [0x7fda8cd68eb7]
01:50:49.322 5644   4   0x7fda8cca6d4f /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37(_ZN7WebCore12ThreadTimers24sharedTimerFiredInternalEv+0xaf) [0x7fda8cca6d4f]
01:50:49.323 5644   5   0x7fda8abc4a55 /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18(_ZN3WTF15GMainLoopSource12voidCallbackEv+0x295) [0x7fda8abc4a55]
01:50:49.323 5644   6   0x7fda8abc300a /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18(_ZN3WTF15GMainLoopSource18voidSourceCallbackEPS0_+0xa) [0x7fda8abc300a]
01:50:49.323 5644   7   0x7fda8abc304e /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18(+0x85f04e) [0x7fda8abc304e]
01:50:49.323 5644   8   0x7fda87feca26 /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/DependenciesGTK/Root/lib64/libglib-2.0.so.0(+0x53a26) [0x7fda87feca26]
01:50:49.323 5644   9   0x7fda87fed854 /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/DependenciesGTK/Root/lib64/libglib-2.0.so.0(g_main_context_dispatch+0x33) [0x7fda87fed854]
01:50:49.323 5644   10  0x7fda87feda39 /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/DependenciesGTK/Root/lib64/libglib-2.0.so.0(+0x54a39) [0x7fda87feda39]
01:50:49.323 5644   11  0x7fda87fede60 /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/DependenciesGTK/Root/lib64/libglib-2.0.so.0(g_main_loop_run+0x1d7) [0x7fda87fede60]
01:50:49.323 5644   12  0x7fda8d8e98a0 /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37(_ZN3WTF7RunLoop3runEv+0x120) [0x7fda8d8e98a0]
01:50:49.323 5644   13  0x7fda8c4551f2 /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37(WebProcessMainUnix+0x1e2) [0x7fda8c4551f2]
01:50:49.323 5644   14  0x7fda83f2ab45 /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf5) [0x7fda83f2ab45]
01:50:49.323 5644   15  0x400c25 /home/slave/webkitgtk/gtk-linux-64-release-tests/build/WebKitBuild/Release/bin/WebKitWebProcess() [0x400c25]
01:50:49.326 5566 [16963/37088] fast/mediastream/MediaStreamTrackEvent-constructor.html failed unexpectedly (WebProcess crashed)

[Carlos Garcia Campos]

I skipped the tests for now in r190266. I don't have time to look at it and I don't even build with media stream enabled.

[Michael Catanzaro]

Calvaris, Eric, should we roll this out, since it broke at least one cross-platform test as well, or can you fix it?

[Eric Carlson]

(In reply to comment #1)
> It seems some layout tests are also affected:
> 
> 01:50:49.322 5644 worker/1
> fast/mediastream/MediaStreamTrackEvent-constructor.html crashed, (stderr
> lines):
> 01:50:49.322 5644   1   0x7fda8ab83687
> /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/Release/lib/
> libjavascriptcoregtk-4.0.so.18(WTFCrash+0x17) [0x7fda8ab83687]
> 01:50:49.322 5644   2   0x7fda365792f5
> /home/slave/webkitgtk/gtk-linux-64-release-tests/build/WebKitBuild/Release/
> lib/libTestRunnerInjectedBundle.so(+0xd62f5) [0x7fda365792f5]
> 01:50:49.322 5644   3   0x7fda8cd68eb7
> /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/Release/lib/
> libwebkit2gtk-4.0.so.37(_ZN7WebCore10TimerEvent10timerFiredEv+0x17)
> [0x7fda8cd68eb7]
> 01:50:49.322 5644   4   0x7fda8cca6d4f
> /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/Release/lib/
> libwebkit2gtk-4.0.so.
> 37(_ZN7WebCore12ThreadTimers24sharedTimerFiredInternalEv+0xaf)
> [0x7fda8cca6d4f]
> 01:50:49.323 5644   5   0x7fda8abc4a55
> /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/Release/lib/
> libjavascriptcoregtk-4.0.so.
> 18(_ZN3WTF15GMainLoopSource12voidCallbackEv+0x295) [0x7fda8abc4a55]
> 01:50:49.323 5644   6   0x7fda8abc300a
> /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/Release/lib/
> libjavascriptcoregtk-4.0.so.
> 18(_ZN3WTF15GMainLoopSource18voidSourceCallbackEPS0_+0xa) [0x7fda8abc300a]
> 01:50:49.323 5644   7   0x7fda8abc304e
> /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/Release/lib/
> libjavascriptcoregtk-4.0.so.18(+0x85f04e) [0x7fda8abc304e]
> 01:50:49.323 5644   8   0x7fda87feca26
> /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/DependenciesGTK/
> Root/lib64/libglib-2.0.so.0(+0x53a26) [0x7fda87feca26]
> 01:50:49.323 5644   9   0x7fda87fed854
> /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/DependenciesGTK/
> Root/lib64/libglib-2.0.so.0(g_main_context_dispatch+0x33) [0x7fda87fed854]
> 01:50:49.323 5644   10  0x7fda87feda39
> /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/DependenciesGTK/
> Root/lib64/libglib-2.0.so.0(+0x54a39) [0x7fda87feda39]
> 01:50:49.323 5644   11  0x7fda87fede60
> /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/DependenciesGTK/
> Root/lib64/libglib-2.0.so.0(g_main_loop_run+0x1d7) [0x7fda87fede60]
> 01:50:49.323 5644   12  0x7fda8d8e98a0
> /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/Release/lib/
> libwebkit2gtk-4.0.so.37(_ZN3WTF7RunLoop3runEv+0x120) [0x7fda8d8e98a0]
> 01:50:49.323 5644   13  0x7fda8c4551f2
> /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/Release/lib/
> libwebkit2gtk-4.0.so.37(WebProcessMainUnix+0x1e2) [0x7fda8c4551f2]
> 01:50:49.323 5644   14  0x7fda83f2ab45
> /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf5) [0x7fda83f2ab45]
> 01:50:49.323 5644   15  0x400c25
> /home/slave/webkitgtk/gtk-linux-64-release-tests/build/WebKitBuild/Release/
> bin/WebKitWebProcess() [0x400c25]
> 01:50:49.326 5566 [16963/37088]
> fast/mediastream/MediaStreamTrackEvent-constructor.html failed unexpectedly
> (WebProcess crashed)

I see this crash:
WTFCrash + 39 (Assertions.cpp:321)
WebCore::Supplementable<WebCore::Page>::provideSupplement(char const*, std::__1::unique_ptr<WebCore::Supplement<WebCore::Page>, std::__1::default_delete<WebCore::Supplement<WebCore::Page> > >) + 146 (Supplementable.h:102)
WebCore::Supplement<WebCore::Page>::provideTo(WebCore::Supplementable<WebCore::Page>*, char const*, std::__1::unique_ptr<WebCore::Supplement<WebCore::Page>, std::__1::default_delete<WebCore::Supplement<WebCore::Page> > >) + 424 (Supplementable.h:87)
WebCore::provideUserMediaTo(WebCore::Page*, WebCore::UserMediaClient*) + 700 (UserMediaController.cpp:49)
WebCore::Internals::Internals(WebCore::Document*) + 630 (Internals.cpp:365)
WebCore::Internals::Internals(WebCore::Document*) + 29 (Internals.cpp:365)

Is this crash new with r190262, or could it have been caused by r190061?

[Tim Horton]

(In reply to comment #4)
> (In reply to comment #1)
> Is this crash new with r190262, or could it have been caused by r190061?

That seems most likely to have come from r190061. I'm not sure why, since the only client who should be installing more than one UserMediaController supplement is Internals, and the Internals code should be deleting the old one before installing a new one...

[Eric Carlson]

Created attachment 262038 [details]
Patch for the bots.

[Darin Adler]

Comment on attachment 262038 [details]
Patch for the bots.

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

> Source/WebCore/platform/mock/MockRealtimeMediaSourceCenter.cpp:148
> +        MockSourceMap::iterator it = map.find(mockAudioSourceID());
> +        ASSERT(it != map.end());
> +
> +        RefPtr<RealtimeMediaSource> audioSource = it->value;

Normally we’d use get here instead of find.

    ASSERT(!map.contains(mockAudioSourceID());
    RefPtr<RealtimeMediaSource> audioSource = map.get(mockAudioSourceID());

> Source/WebCore/platform/mock/MockRealtimeMediaSourceCenter.cpp:163
> +        MockSourceMap::iterator it = map.find(mockVideoSourceID());
> +        ASSERT(it != map.end());
> +
> +        RefPtr<RealtimeMediaSource> videoSource = it->value;

Ditto.

> Source/WebCore/platform/mock/TimerEventBasedMock.h:71
> +        m_mock = nullptr;

I don’t understand the value of this. We are about to delete m_mock, so how is setting it to nullptr valuable? Is it about debugging?

[Eric Carlson]

Comment on attachment 262038 [details]
Patch for the bots.

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

Thanks for the review. I will hold off on committing this until I hear back from Adam about whether or not this causes problems for the GTK port.

>> Source/WebCore/platform/mock/MockRealtimeMediaSourceCenter.cpp:148
>> +        RefPtr<RealtimeMediaSource> audioSource = it->value;
> 
> Normally we’d use get here instead of find.
> 
>     ASSERT(!map.contains(mockAudioSourceID());
>     RefPtr<RealtimeMediaSource> audioSource = map.get(mockAudioSourceID());

OK.

>> Source/WebCore/platform/mock/MockRealtimeMediaSourceCenter.cpp:163
>> +        RefPtr<RealtimeMediaSource> videoSource = it->value;
> 
> Ditto.

Ditto.

>> Source/WebCore/platform/mock/TimerEventBasedMock.h:71
>> +        m_mock = nullptr;
> 
> I don’t understand the value of this. We are about to delete m_mock, so how is setting it to nullptr valuable? Is it about debugging?

Oops, I did add that while debugging. I will remove it.

[Adam Bergkvist]

Comment on attachment 262038 [details]
Patch for the bots.

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

> Source/WebCore/platform/mediastream/mac/RealtimeMediaSourceCenterMac.cpp:87
> +    client->constraintsValidated(audioSources, videoSources);

This change flips the order of video and audio.

We usually have audio first (like in this patch), but UserMediaRequest::constraintsValidated() has the oposite order.

> Source/WebCore/platform/mediastream/openwebrtc/RealtimeMediaSourceCenterOwr.cpp:200
> +    m_client->constraintsValidated(audioSources, videoSources);

Argument order flipped (see previous comment)

> Source/WebCore/platform/mock/MockRealtimeMediaSourceCenter.cpp:168
> +    client->constraintsValidated(audioSources, videoSources);

Argument order flipped (see previous comment)

[Adam Bergkvist]

Fixing the argument order mentioned above and unprefixing the MediaStream constructor (s/webkitMediaStream/MediaStream/) in the tests make all but 4 tests skipped in [1] and [2] pass.

[1] http://trac.webkit.org/changeset/190266/trunk/LayoutTests/platform/gtk/TestExpectations

[2] http://trac.webkit.org/changeset/189089/trunk/LayoutTests/platform/gtk/TestExpectations

[Eric Carlson]

Created attachment 262168 [details]
Patch for landing.


> > Source/WebCore/platform/mediastream/mac/RealtimeMediaSourceCenterMac.cpp:87
> > +    client->constraintsValidated(audioSources, videoSources);
> 
> This change flips the order of video and audio.
> 
> We usually have audio first (like in this patch), but UserMediaRequest::constraintsValidated() has the oposite order.
> 
> > Source/WebCore/platform/mediastream/openwebrtc/RealtimeMediaSourceCenterOwr.cpp:200
> > +    m_client->constraintsValidated(audioSources, videoSources);
> 
> Argument order flipped (see previous comment)
> 
> > Source/WebCore/platform/mock/MockRealtimeMediaSourceCenter.cpp:168
> > +    client->constraintsValidated(audioSources, videoSources);
> 
> Argument order flipped (see previous comment)
> 
Fixed the MediaStreamCreationClient::constraintsValidated argument order so hopefully it is audio,video everywhere. 


> Fixing the argument order mentioned above and unprefixing the MediaStream constructor (s/webkitMediaStream/MediaStream/) in the tests make all but 4 tests skipped in [1] and [2] pass.
> 
> [1] http://trac.webkit.org/changeset/190266/trunk/LayoutTests/platform/gtk/TestExpectations
> 
> [2] http://trac.webkit.org/changeset/189089/trunk/LayoutTests/platform/gtk/TestExpectations
> 
I didn't unprefix the MediaStream constructor in this patch, I will do it in a followup. I don't know which tests didn't work for Adam, but I unskipped them all with the assumption that it will be easier to skip the ones that still fail once the bots point out which still have problems.

[WebKit Commit Bot]

Comment on attachment 262168 [details]
Patch for landing.

Rejecting attachment 262168 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 262168, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Tools/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/227708

[Eric Carlson]

Created attachment 262185 [details]
Updated patch for landing.

[WebKit Commit Bot]

Comment on attachment 262185 [details]
Updated patch for landing.

Clearing flags on attachment: 262185

Committed r190362: <http://trac.webkit.org/changeset/190362>

[Carlos Garcia Campos]

Layout tests no longer crash, but the GTK+ unit tests are still failing. Could you look at it, phil?

[ChangSeok Oh]

Created attachment 263468 [details]
Patch

[ChangSeok Oh]

(In reply to comment #15)
> Layout tests no longer crash, but the GTK+ unit tests are still failing.
> Could you look at it, phil?

The failures are no longer reproduced.

[Carlos Garcia Campos]

Comment on attachment 263468 [details]
Patch

Ok. Thanks!

[WebKit Commit Bot]

Comment on attachment 263468 [details]
Patch

Clearing flags on attachment: 263468

Committed r191298: <http://trac.webkit.org/changeset/191298>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.