Bug 149580

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 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.

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

(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?

(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...

Created attachment 262038 [details] Patch for the bots.

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?

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.

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)

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

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.

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

Created attachment 262185 [details] Updated patch for landing.

Comment on attachment 262185 [details] Updated patch for landing. Clearing flags on attachment: 262185 Committed r190362: <http://trac.webkit.org/changeset/190362>

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

Created attachment 263468 [details] Patch

(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.

Comment on attachment 263468 [details] Patch Ok. Thanks!

Comment on attachment 263468 [details] Patch Clearing flags on attachment: 263468 Committed r191298: <http://trac.webkit.org/changeset/191298>

All reviewed patches have been landed. Closing bug.