1181262 - --disable-webrtc builds are busted with "RTCCertificate.h:23:10: fatal error: 'mtransport/dtlsidentity.h' file not found"

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P1)

Description

[Daniel Holbert [:dholbert]]

STR:
Try to build with this in your mozconfig:
  ac_add_options --disable-webrtc

ACTUAL RESULTS:
 1:31.47 In file included from /scratch/work/builds/mozilla-inbound/obj/dom/media/webrtc/Unified_cpp_dom_media_webrtc0.cpp:20:
 1:31.47 In file included from /scratch/work/builds/mozilla-inbound/mozilla/dom/media/webrtc/RTCCertificate.cpp:7:
 1:31.47 ../../../dist/include/mozilla/dom/RTCCertificate.h:23:10: fatal error: 'mtransport/dtlsidentity.h' file not found
 1:31.47 #include "mtransport/dtlsidentity.h"
 1:31.47          ^
 1:31.55 1 error generated.

Looks like the RTCCertificate files were just recently added in bug 1172785, so this is a regression from that bug.

(Note also that --disable-webrtc builds had different, unrelated bustage introduced a couple days ago; bug 1180748 just fixed that, but now we're left with this new bustage.)

Comment 1

[Daniel Holbert [:dholbert]]

Note that dom/media/webrtc/moz.build has two sections of UNIFIED_SOURCES -- one of which is guarded with MOZ_WEBRTC, and one of which is unconditional.

RTCCertificate.cpp is in the latter unconditional section right now. Perhaps it belongs in the former MOZ_WEBRTC-specific section?

Comment 2

[Daniel Holbert [:dholbert]]

Hmm, comment 1 isn't quite sufficient. It does seem to get us past this issue for RTCCertificate.cpp [because we don't build that file anymore].  But that's not the only file that includes RTCCertificate.h, so we hit the same issue later on.

In particular -- I also hit this error when building RTCCertificateBinding.cpp (as part of obj/dom/bindings/UnifiedBindings14.cpp).

It also looks like this header is included (and this class is instantiated) in dom/base/nsJSEnvironment.cpp, so we need some #ifdeffing there probably.

Comment 3

[Martin Thomson [:mt:]]

Attachment: MozReview Request: Bug 1181262 - Disabling more code under --disable-webrtc, r=dholbert,bwc

Bug 1181262 - Disabling more code under --disable-webrtc, r=dholbert,bwc

Comment 4

[Martin Thomson [:mt:]]

We should really stop supporting these "don't build X" options.  I'm sure that it saves some time, but not enough to warrant this sort of pain.  In any case, I've a patch.

Comment 5

[Byron Campen [:bwc]]

Comment on attachment 8631089 [details]
MozReview Request: Bug 1181262 - Disabling more code under --disable-webrtc, r=dholbert,bwc

https://reviewboard.mozilla.org/r/12837/#review11413

::: dom/base/nsJSEnvironment.cpp:2553
(Diff revision 1)
>    if (tag == SCTAG_DOM_RTC_CERTIFICATE) {
> +#ifdef MOZ_WEBRTC
>      nsIGlobalObject *global = xpc::NativeGlobal(JS::CurrentGlobalOrNull(cx));
>      if (!global) {
>        return nullptr;
>      }
>  
>      // Prevent the return value from being trashed by a GC during ~nsRefPtr.
>      JS::Rooted<JSObject*> result(cx);
>      {
>        nsRefPtr<RTCCertificate> cert = new RTCCertificate(global);
>        if (!cert->ReadStructuredClone(reader)) {
>          result = nullptr;
>        } else {
>          result = cert->WrapObject(cx, nullptr);
>        }
>      }
>      return result;
> +#else
> +    return nullptr;
> +#endif

Looking at the original patch, it seems that a build with webrtc disabled will differ in behavior from a build before this webrtc certs stuff was implemented. Is this intended? Should this whole thing be wrapped in #ifdef?

::: js/xpconnect/src/Sandbox.cpp:40
(Diff revision 1)
> +#ifdef MOZ_WEBRTC
>  #include "mozilla/dom/RTCIdentityProviderRegistrar.h"
> +#endif

I'm guessing this is fallout from the extra build system cleanup?

Comment 6

[Martin Thomson [:mt:]]

https://reviewboard.mozilla.org/r/12837/#review11413

> Looking at the original patch, it seems that a build with webrtc disabled will differ in behavior from a build before this webrtc certs stuff was implemented. Is this intended? Should this whole thing be wrapped in #ifdef?

I'm only copying what was already there for the MOZ_NFC thing above.  In that case, the functional difference is that the method returns nullptr and doesn't throw also.  The upshot of this being that stored values are turned into null; rather than causing errors.  I'm not going to try to defend the merits of that choice, but it seems least disruptive.

To be perfectly clear, I don't actually care what happens when MOZ_WEBRTC is not defined.  Frankly, I'd rather not have the directive at all.

> I'm guessing this is fallout from the extra build system cleanup?

Yeah, I started by moving all the webrtc objects under the conditional; this is one of them.  It seemed like it would be better to include them all rather than do things piecemeal.  I ultimately left RTCStatsReport, since I didn't want to play with the ipdl generation too much.  This could be given the same treatment, but I'm far more confident about changing this code, since I wrote it in the first place.

Comment 7

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/12837/#review11413

> I'm only copying what was already there for the MOZ_NFC thing above.  In that case, the functional difference is that the method returns nullptr and doesn't throw also.  The upshot of this being that stored values are turned into null; rather than causing errors.  I'm not going to try to defend the merits of that choice, but it seems least disruptive.
> 
> To be perfectly clear, I don't actually care what happens when MOZ_WEBRTC is not defined.  Frankly, I'd rather not have the directive at all.

Good enough for me.

Comment 8

[Byron Campen [:bwc]]

Comment on attachment 8631089 [details]
MozReview Request: Bug 1181262 - Disabling more code under --disable-webrtc, r=dholbert,bwc

https://reviewboard.mozilla.org/r/12837/#review11419

Comment 9

[Daniel Holbert [:dholbert]]

https://reviewboard.mozilla.org/r/12835/#review11433

::: dom/media/webrtc/RTCIdentityProviderRegistrar.cpp:1
(Diff revision 1)
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-*/

[This is my first time using ReviewBoard; apologies if I do something stupid/wrong]

Looks like you're entirely deleting this .h and .cpp file, and then recreating them as brand-new files in a new location.

I think you really want to use "hg mv" aka "hg rename" to move the files. (This lets hg be smarter about the file-history, e.g. preserving hg blame.)

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 8631089 [details]
MozReview Request: Bug 1181262 - Disabling more code under --disable-webrtc, r=dholbert,bwc

I verified that the patch fixes my build, and the #ifdeffing seems sane.

r=me as long as you use "hg mv" to move the .h/.cpp files instead of deleting & recreating them. (So, the patch file should say "rename from ... rename to ..." and not show any actual diffs for those files, except for any contents that have actually changed.)

Comment 11

[Martin Thomson [:mt:]]

Comment on attachment 8631089 [details]
MozReview Request: Bug 1181262 - Disabling more code under --disable-webrtc, r=dholbert,bwc

Bug 1181262 - Disabling more code under --disable-webrtc, r=dholbert,bwc

Comment 12

[Martin Thomson [:mt:]]

I failed to get the build working properly without --disable-webrtc, so I'm going to wait until try comes back green before I land this.

Comment 13

[Martin Thomson [:mt:]]

OK, enough green:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e8193deffea

Comment 14

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Martin Thomson [:mt:] from comment #4)
> We should really stop supporting these "don't build X" options.  I'm sure
> that it saves some time, but not enough to warrant this sort of pain.  In
> any case, I've a patch.

People actually build this.  A few to make builds faster (though it's not much), but some distributions do as well - Tor IIRC, but also a few others (IceWeasel?), and IIRC the builds of gecko for Thunderbird

Comment 15

[Eric Rescorla (:ekr)]

I'm sort of sensitive to the Thunderbird issue, but is there a reason Tor etc. can't just pref it off?

Comment 16

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Eric Rescorla (:ekr) from comment #15)
> I'm sort of sensitive to the Thunderbird issue, but is there a reason Tor
> etc. can't just pref it off?

Some don't trust it.  (Not sure if Tor falls in that camp)  For people who pref it off, it's bloat (though we don't have to care about that - Thunderbird is an example perhaps. people trying to "light" versions of FF too).  For some it may be semi-political.  For others they haven't considered that option - they know they don't want it, and it's as easy (or easier really) to modify the build params and be *sure* as it is to change the default prefs, and perhaps issues with old/shared prefs.

That said - it'd be handy to drop the disable support now (though as mentioned Thunderbird and similar cases might be a valid argument for keeping it).

Comment 17

[Martin Thomson [:mt:]]

Well, this patch improves the --disable-webrtc story a little.  We compile slightly less now.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0e8193deffea