1425996 - Various builds will be busted when Gecko 59 merges to Beta on 2018-01-11

Status: VERIFIED FIXED

(Core :: WebRTC: Signaling, defect, P1)

Description

[Ryan VanderMeulen [:RyanVM]]

[Tracking Requested - why for this release]: Broken builds on the next merge day.

I believe MOZ_DIAGNOSTIC_ASSERT usage needs to be wrapped in MOZ_DIAGNOSTIC_ASSERT_ENABLED ifdef guards?

https://treeherder.mozilla.org/logviewer.html#?job_id=150903173&repo=try

/builds/worker/workspace/build/src/media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:211:14: error: unused variable 'rv' [-Werror,-Wunused-variable]

Comment 1

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

The early/first merge will be Jan. 11th. 
Tracking so we can make sure to check back in early January.

Comment 2

[Ryan VanderMeulen [:RyanVM]]

JW, maybe you can help? DebugOnly isn't an option because MOZ_DIAGNOSTIC_ASSERT is used on opt builds as well. But I'm not sure exactly how to MOZ_DIAGNOSTIC_ASSERT_ENABLED ifdef my way to victory here either given how rv is being assigned in the first place.

Comment 3

[Ryan VanderMeulen [:RyanVM]]

Anyway, this is breaking nearly all the builds at the moment and actively blocking me running useful simulations :(

Comment 4

[JW Wang [:jwwang] [:jw_wang]]

Checking...

Comment 5

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Bug 1425996 - fix build error: unused variable 'rv'.

Review commit: https://reviewboard.mozilla.org/r/209002/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/209002/

Comment 6

[Gerald Squelart (he/him)]

Comment on attachment 8938250 [details]
Bug 1425996 - fix build error: unused variable 'rv'.

https://reviewboard.mozilla.org/r/209002/#review214690

::: dom/media/MediaDecoder.cpp:1232
(Diff revision 1)
>    nsresult rv = mReader->OwnerThread()->Dispatch(
>      NewRunnableMethod("MediaFormatReader::NotifyDataArrived",
>                        mReader.get(),
>                        &MediaFormatReader::NotifyDataArrived));
>    MOZ_DIAGNOSTIC_ASSERT(NS_SUCCEEDED(rv));
> +  Unused << rv;

LGTM.

But could you please check that non-unified build still works (for Chris' bug 1388605).

Comment 7

[JW Wang [:jwwang] [:jw_wang]]

It is sad that disabling unified-build for the entire dom/media folder causes build errors even for files not changed at all. I will land the patch and fix build errors for servo build if any.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/06a19fbe2581
fix build error: unused variable 'rv'. r=gerald

Comment 9

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/06a19fbe2581

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Still busted :(

https://treeherder.mozilla.org/logviewer.html#?job_id=153041955&repo=try is off current m-c tip (rev f844e99f26b1)

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Oh, this is in the other spot in MediaPipeline.cpp. I'll try giving it the same treatment that the first location got.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #11)
> Oh, this is in the other spot in MediaPipeline.cpp. I'll try giving it the
> same treatment that the first location got.

Which just moves the bustage to ChannelMediaDecoder.cpp :(

Comment 13

[Nils Ohlmeier [:drno]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #12)
> Which just moves the bustage to ChannelMediaDecoder.cpp :(

At least that is the last file in dom/media which doesn't have the Unused << after the MOZ_DIAGNOSTIC_ASSERT(NS_SUCCEEDED(rv)) calls :-)

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Looks like those are indeed the only spots still needing patching. Going to land it with Gerald's previous r+.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc335a5f19e8
Add in a few more instances of Unused that got missed in the first patch. r=gerald

Comment 16

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/bc335a5f19e8