Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1982643002:
Fix backwards WebRTC-in-Chromium override and expose Chromium logging setup (Closed)
DescriptionFix backwards WebRTC-in-Chromium override, and expose a way for WebRTC code to initialise the Chromium logging system.
BUG=561667
Committed: https://crrev.com/85d72ea76ae71e4b9d71d990e67c9d6b3cc1d145
Cr-Commit-Position: refs/heads/master@{#394384}
Patch Set 1 #Patch Set 2 : Add a way for WebRTC code to initialise Chromium logging #Patch Set 3 : ...and expose it even if !defined(LOGGING_INSIDE_WEBRTC) #
Total comments: 6
Patch Set 4 : More comments and a new case(LS_NONE) #
Total comments: 4
Messages
Total messages: 24 (7 generated)
Description was changed from ========== Fix backwards WebRTC-in-Chromium override BUG= ========== to ========== Fix backwards WebRTC-in-Chromium override BUG= ==========
Description was changed from ========== Fix backwards WebRTC-in-Chromium override BUG= ========== to ========== Fix backwards WebRTC-in-Chromium override, and expose a way for WebRTC code to initialise the Chromium logging system. BUG=561667 ==========
katrielc@chromium.org changed reviewers: + grunell@chromium.org
I'm not super confident about these changes -- in particular, am surprised that LogToDebug has been working so far. I suspect it isn't called very often.
https://codereview.chromium.org/1982643002/diff/40001/third_party/webrtc_over... File third_party/webrtc_overrides/webrtc/base/diagnostic_logging.h (right): https://codereview.chromium.org/1982643002/diff/40001/third_party/webrtc_over... third_party/webrtc_overrides/webrtc/base/diagnostic_logging.h:60: enum LoggingSeverity { LS_NONE = 0, Describe this in the comment. https://codereview.chromium.org/1982643002/diff/40001/third_party/webrtc_over... File third_party/webrtc_overrides/webrtc/base/logging.cc (right): https://codereview.chromium.org/1982643002/diff/40001/third_party/webrtc_over... third_party/webrtc_overrides/webrtc/base/logging.cc:217: logging::SetMinLogLevel(WebRtcSevToChromeSev(min_sev)); Yes, this was indeed wrong. Great to have it fixed! https://codereview.chromium.org/1982643002/diff/40001/third_party/webrtc_over... File third_party/webrtc_overrides/webrtc/base/logging.h (right): https://codereview.chromium.org/1982643002/diff/40001/third_party/webrtc_over... third_party/webrtc_overrides/webrtc/base/logging.h:29: // Chromium allows a minimum programmatically-set log level of -1 Explain that this function is only used when WebRTC standalone is built but it used the Chromium logging. Add TODO for removing it when the fuzzers are built/run on WebRTC bots, i.e. not inside Chromium. (Sorry for any incorrect/confused terminology...:)) Link to bug.
https://codereview.chromium.org/1982643002/diff/40001/third_party/webrtc_over... File third_party/webrtc_overrides/webrtc/base/diagnostic_logging.h (right): https://codereview.chromium.org/1982643002/diff/40001/third_party/webrtc_over... third_party/webrtc_overrides/webrtc/base/diagnostic_logging.h:60: enum LoggingSeverity { LS_NONE = 0, On 2016/05/18 08:43:26, Henrik Grunell wrote: > Describe this in the comment. Acknowledged. https://codereview.chromium.org/1982643002/diff/40001/third_party/webrtc_over... File third_party/webrtc_overrides/webrtc/base/logging.h (right): https://codereview.chromium.org/1982643002/diff/40001/third_party/webrtc_over... third_party/webrtc_overrides/webrtc/base/logging.h:29: // Chromium allows a minimum programmatically-set log level of -1 On 2016/05/18 08:43:26, Henrik Grunell wrote: > Add TODO for removing it when the fuzzers are built/run on WebRTC bots, i.e. not > inside Chromium. I checked with phoglund@ and they don't plan to move the ClusterFuzz uploads to the WebRTC bot, hence probably it makes sense not to add a TODO/bug link.
lgtm https://codereview.chromium.org/1982643002/diff/40001/third_party/webrtc_over... File third_party/webrtc_overrides/webrtc/base/logging.h (right): https://codereview.chromium.org/1982643002/diff/40001/third_party/webrtc_over... third_party/webrtc_overrides/webrtc/base/logging.h:29: // Chromium allows a minimum programmatically-set log level of -1 On 2016/05/18 09:06:42, katrielc wrote: > On 2016/05/18 08:43:26, Henrik Grunell wrote: > > Add TODO for removing it when the fuzzers are built/run on WebRTC bots, i.e. > not > > inside Chromium. > > I checked with phoglund@ and they don't plan to move the ClusterFuzz uploads to > the WebRTC bot, hence probably it makes sense not to add a TODO/bug link. Acknowledged.
The CQ bit was checked by katrielc@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1982643002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1982643002/60001
Message was sent while issue was closed.
Description was changed from ========== Fix backwards WebRTC-in-Chromium override, and expose a way for WebRTC code to initialise the Chromium logging system. BUG=561667 ========== to ========== Fix backwards WebRTC-in-Chromium override, and expose a way for WebRTC code to initialise the Chromium logging system. BUG=561667 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Fix backwards WebRTC-in-Chromium override, and expose a way for WebRTC code to initialise the Chromium logging system. BUG=561667 ========== to ========== Fix backwards WebRTC-in-Chromium override, and expose a way for WebRTC code to initialise the Chromium logging system. BUG=561667 Committed: https://crrev.com/85d72ea76ae71e4b9d71d990e67c9d6b3cc1d145 Cr-Commit-Position: refs/heads/master@{#394384} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/85d72ea76ae71e4b9d71d990e67c9d6b3cc1d145 Cr-Commit-Position: refs/heads/master@{#394384}
Message was sent while issue was closed.
tommi@chromium.org changed reviewers: + tommi@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1982643002/diff/60001/third_party/webrtc_over... File third_party/webrtc_overrides/webrtc/base/logging.cc (right): https://codereview.chromium.org/1982643002/diff/60001/third_party/webrtc_over... third_party/webrtc_overrides/webrtc/base/logging.cc:54: void InitChromiumLoggingAndCommandLine() { this function shouldn't be here - is it unused? https://codereview.chromium.org/1982643002/diff/60001/third_party/webrtc_over... File third_party/webrtc_overrides/webrtc/base/logging.h (right): https://codereview.chromium.org/1982643002/diff/60001/third_party/webrtc_over... third_party/webrtc_overrides/webrtc/base/logging.h:35: void InitChromiumLoggingAndCommandLine(); nit: we don't indent contents of namespaces. https://codereview.chromium.org/1982643002/diff/60001/third_party/webrtc_over... third_party/webrtc_overrides/webrtc/base/logging.h:36: } nit: } // namespace rtc
Message was sent while issue was closed.
https://codereview.chromium.org/1982643002/diff/60001/third_party/webrtc_over... File third_party/webrtc_overrides/webrtc/base/logging.cc (right): https://codereview.chromium.org/1982643002/diff/60001/third_party/webrtc_over... third_party/webrtc_overrides/webrtc/base/logging.cc:54: void InitChromiumLoggingAndCommandLine() { On 2016/05/18 11:47:50, tommi-chrömium wrote: > this function shouldn't be here - is it unused? It's currently unused, but the plan is to call it from webrtc (specifically, under an ifdef WEBRTC_CHROMIUM_BUILD in webrtc_fuzzer_main.cc). If not here, where should it be?
Message was sent while issue was closed.
On 2016/05/18 12:00:46, katrielc wrote: > https://codereview.chromium.org/1982643002/diff/60001/third_party/webrtc_over... > File third_party/webrtc_overrides/webrtc/base/logging.cc (right): > > https://codereview.chromium.org/1982643002/diff/60001/third_party/webrtc_over... > third_party/webrtc_overrides/webrtc/base/logging.cc:54: void > InitChromiumLoggingAndCommandLine() { > On 2016/05/18 11:47:50, tommi-chrömium wrote: > > this function shouldn't be here - is it unused? > > It's currently unused, but the plan is to call it from webrtc (specifically, > under an ifdef WEBRTC_CHROMIUM_BUILD in webrtc_fuzzer_main.cc). > > If not here, where should it be? This is code that ships with Chrome, so if it's only used for tests, it needs to be in test code. Why does the global command line need to be modified btw?
Message was sent while issue was closed.
On 2016/05/18 13:42:24, tommi-chrömium wrote: > This is code that ships with Chrome, so if it's only used for tests, it needs to > be in test code. Ah, ok, my bad. Should I revert this commit? I can discuss with grunell@ tomorrow. > Why does the global command line need to be modified btw? Without changing the global command line, g_vlog_info won't be set by InitLogging, so logging::GetVlogLevelHelper returns max(-1, level). Hence WebRTC code using Chromium logging can't set the log level to -2 to disable ERROR messages. I didn't see a better way to do this without writing a special case into Chromium, but there may well be one.
Message was sent while issue was closed.
On 2016/05/18 13:42:24, tommi-chrömium wrote: > This is code that ships with Chrome, so if it's only used for tests, it needs to > be in test code. Ah, ok, my bad. Should I revert this commit? I can discuss with grunell@ tomorrow. > Why does the global command line need to be modified btw? Without changing the global command line, g_vlog_info won't be set by InitLogging, so logging::GetVlogLevelHelper returns max(-1, level). Hence WebRTC code using Chromium logging can't set the log level to -2 to disable ERROR messages. I didn't see a better way to do this without writing a special case into Chromium, but there may well be one.
Message was sent while issue was closed.
On 2016/05/18 14:34:44, katrielc wrote: > On 2016/05/18 13:42:24, tommi-chrömium wrote: > > This is code that ships with Chrome, so if it's only used for tests, it needs > to > > be in test code. > Ah, ok, my bad. Should I revert this commit? > > I can discuss with grunell@ tomorrow. > > > Why does the global command line need to be modified btw? > > Without changing the global command line, g_vlog_info won't be set by > InitLogging, so logging::GetVlogLevelHelper returns max(-1, level). Hence WebRTC > code using Chromium logging can't set the log level to -2 to disable ERROR > messages. > > I didn't see a better way to do this without writing a special case into > Chromium, but there may well be one. Yeah, the problem is that the WebRTC libfuzzer fuzzers are built by a Chrome bot, which builds off third_party/webrtc and uses the Chromium logging system instead of the WebRTC one. Without this change it's not possible to disable logging, like Katriel says. It is pretty hacky but I don't know any better way to do it; I don't know of any test code where we can put it. We did discuss building these off a WebRTC bot, but that requires an unknown amount of work + maintenance so we'd rather not do that at this point. How unacceptable do you think this is? :)
Message was sent while issue was closed.
On 2016/05/18 14:55:56, phoglund_chrome wrote: > On 2016/05/18 14:34:44, katrielc wrote: > > On 2016/05/18 13:42:24, tommi-chrömium wrote: > > > This is code that ships with Chrome, so if it's only used for tests, it > needs > > to > > > be in test code. > > Ah, ok, my bad. Should I revert this commit? > > > > I can discuss with grunell@ tomorrow. > > > > > Why does the global command line need to be modified btw? > > > > Without changing the global command line, g_vlog_info won't be set by > > InitLogging, so logging::GetVlogLevelHelper returns max(-1, level). Hence > WebRTC > > code using Chromium logging can't set the log level to -2 to disable ERROR > > messages. > > > > I didn't see a better way to do this without writing a special case into > > Chromium, but there may well be one. > > Yeah, the problem is that the WebRTC libfuzzer fuzzers are built by a Chrome > bot, > which builds off third_party/webrtc and uses the Chromium logging system instead > of the WebRTC one. Without this change it's not possible to disable logging, > like > Katriel says. It is pretty hacky but I don't know any better way to do it; I > don't > know of any test code where we can put it. > > We did discuss building these off a WebRTC bot, but that requires an unknown > amount of work + maintenance so we'd rather not do that at this point. How > unacceptable do you think this is? :) I just had this thought: maybe it's possible to make a fuzzer main in Chromium which includes and instantiates the WebRTC fuzzers? In that case that main can just call the logging and cmdline functions. We'd have to have a separate main in WebRTC so we can run them standalone as well but I think we can live with that.
Message was sent while issue was closed.
On 2016/05/18 14:57:37, phoglund_chrome wrote:
> On 2016/05/18 14:55:56, phoglund_chrome wrote:
> > On 2016/05/18 14:34:44, katrielc wrote:
> > > On 2016/05/18 13:42:24, tommi-chrömium wrote:
> > > > This is code that ships with Chrome, so if it's only used for tests, it
> > needs
> > > to
> > > > be in test code.
> > > Ah, ok, my bad. Should I revert this commit?
> > >
> > > I can discuss with grunell@ tomorrow.
> > >
> > > > Why does the global command line need to be modified btw?
> > >
> > > Without changing the global command line, g_vlog_info won't be set by
> > > InitLogging, so logging::GetVlogLevelHelper returns max(-1, level). Hence
> > WebRTC
> > > code using Chromium logging can't set the log level to -2 to disable ERROR
> > > messages.
> > >
> > > I didn't see a better way to do this without writing a special case into
> > > Chromium, but there may well be one.
> >
> > Yeah, the problem is that the WebRTC libfuzzer fuzzers are built by a Chrome
> > bot,
> > which builds off third_party/webrtc and uses the Chromium logging system
> instead
> > of the WebRTC one. Without this change it's not possible to disable logging,
> > like
> > Katriel says. It is pretty hacky but I don't know any better way to do it; I
> > don't
> > know of any test code where we can put it.
> >
> > We did discuss building these off a WebRTC bot, but that requires an unknown
> > amount of work + maintenance so we'd rather not do that at this point. How
> > unacceptable do you think this is? :)
>
> I just had this thought: maybe it's possible to make a fuzzer main in Chromium
> which includes and instantiates the WebRTC fuzzers? In that case that main can
> just call the logging and cmdline functions. We'd have to have a separate main
> in WebRTC so we can run them standalone as well but I think we can live with
> that.
e.g. make a variant of webrtc_fuzzer_main.cc in Chromium. I don't know
where that build target would live but maybe somewhere close to chromium's
other libfuzzers?
That main doesn't even #include, it just assumes the target will provide an
implementation of FuzzOneInput:
namespace webrtc {
extern void FuzzOneInput(const uint8_t* data, size_t size);
} // namespace webrtc
So fuzzers are very loosely coupled to the WebRTC fuzz main, and it should
be easy to create corresponding targets in Chromium.
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/1992243002/ by katrielc@chromium.org. The reason for reverting is: The signature change for LogToDebug broke JNI on Android..
Message was sent while issue was closed.
OK, I'll chat to him. Do you know if it's possible to get the Chromium bot to do a standalone build of WebRTC? i.e. with BUILD_WITH_CHROMIUM=0? That would solve everything :) On Wed, May 18, 2016 at 10:43 AM <grunell@chromium.org> wrote: > > > https://codereview.chromium.org/1982643002/diff/40001/third_party/webrtc_over... > File third_party/webrtc_overrides/webrtc/base/diagnostic_logging.h > (right): > > > https://codereview.chromium.org/1982643002/diff/40001/third_party/webrtc_over... > third_party/webrtc_overrides/webrtc/base/diagnostic_logging.h:60: enum > LoggingSeverity { LS_NONE = 0, > Describe this in the comment. > > > https://codereview.chromium.org/1982643002/diff/40001/third_party/webrtc_over... > File third_party/webrtc_overrides/webrtc/base/logging.cc (right): > > > https://codereview.chromium.org/1982643002/diff/40001/third_party/webrtc_over... > third_party/webrtc_overrides/webrtc/base/logging.cc:217: > logging::SetMinLogLevel(WebRtcSevToChromeSev(min_sev)); > Yes, this was indeed wrong. Great to have it fixed! > > > https://codereview.chromium.org/1982643002/diff/40001/third_party/webrtc_over... > File third_party/webrtc_overrides/webrtc/base/logging.h (right): > > > https://codereview.chromium.org/1982643002/diff/40001/third_party/webrtc_over... > third_party/webrtc_overrides/webrtc/base/logging.h:29: // Chromium > > allows a minimum programmatically-set log level of -1 > Explain that this function is only used when WebRTC standalone is built > but it used the Chromium logging. > > Add TODO for removing it when the fuzzers are built/run on WebRTC bots, > i.e. not inside Chromium. (Sorry for any incorrect/confused > terminology...:)) Link to bug. > > https://codereview.chromium.org/1982643002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
