Closed
Bug 1181262
Opened 9 years ago
Closed 9 years ago
--disable-webrtc builds are busted with "RTCCertificate.h:23:10: fatal error: 'mtransport/dtlsidentity.h' file not found"
Categories
(Core :: WebRTC, defect, P1)
Core
WebRTC
Tracking
()
RESOLVED
FIXED
mozilla42
Tracking | Status | |
---|---|---|
firefox42 | --- | fixed |
backlog | webrtc/webaudio+ |
People
(Reporter: dholbert, Assigned: mt)
References
Details
(Keywords: regression)
Attachments
(1 file)
(deleted),
text/x-review-board-request
|
Details |
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.)
Reporter | ||
Updated•9 years ago
|
Flags: needinfo?(martin.thomson)
Reporter | ||
Comment 1•9 years ago
|
||
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?
Updated•9 years ago
|
backlog: --- → webRTC+
Rank: 19
Priority: -- → P1
Updated•9 years ago
|
Assignee: nobody → martin.thomson
Reporter | ||
Comment 2•9 years ago
|
||
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.
Assignee | ||
Comment 3•9 years ago
|
||
Bug 1181262 - Disabling more code under --disable-webrtc, r=dholbert,bwc
Attachment #8631089 -
Flags: review?(docfaraday)
Attachment #8631089 -
Flags: review?(dholbert)
Assignee | ||
Comment 4•9 years ago
|
||
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.
Flags: needinfo?(martin.thomson)
Comment 5•9 years ago
|
||
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?
Attachment #8631089 -
Flags: review?(docfaraday)
Assignee | ||
Comment 6•9 years ago
|
||
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•9 years ago
|
||
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•9 years ago
|
||
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
Attachment #8631089 -
Flags: review+
Reporter | ||
Comment 9•9 years ago
|
||
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.)
Reporter | ||
Comment 10•9 years ago
|
||
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.)
Assignee | ||
Updated•9 years ago
|
Attachment #8631089 -
Flags: review?(dholbert)
Attachment #8631089 -
Flags: review+
Assignee | ||
Comment 11•9 years ago
|
||
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
Assignee | ||
Comment 12•9 years ago
|
||
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.
Assignee | ||
Comment 13•9 years ago
|
||
OK, enough green:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e8193deffea
Comment 14•9 years ago
|
||
(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•9 years ago
|
||
I'm sort of sensitive to the Thunderbird issue, but is there a reason Tor etc. can't just pref it off?
Comment 16•9 years ago
|
||
(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).
Assignee | ||
Comment 17•9 years ago
|
||
Well, this patch improves the --disable-webrtc story a little. We compile slightly less now.
Comment 18•9 years ago
|
||
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in
before you can comment on or make changes to this bug.
Description
•