[triage]: emircan, can you take a look?

Thanks. 

chfremer@ this would be a good, small CL to explore logging.

Hi phoglund@. Before attempting a fix, I would like to understand a bit more about the fuzz testing, since I am not yet familiar with it. 

I would like to find an example fuzzer output where I can see the log outputs from the video_frame.cc code site. Could you please point me to where I can find those? Which fuzzer is exercising this code? 

Thanks for your help.

Here is an example: https://cluster-fuzz.appspot.com/testcase?key=4504027371143168

Thanks for the example.
I see in the example log output that the log message is output multiple times.

Lowering the log level does not seem quite right. The log message may actually be relevant for interpreting what is happening during the fuzz test. 

@phoglund, could you help me understand a bit more about how exactly the log message is causing a slow down. Is it increasing the runtime on the cluster? Or is it because it makes the logs harder to interpret for humans?

It's increasing the runtime and taxing the log space for CF I believe. inferno@, can you elaborate?

WebRTC code is exercised by the phoglund_webrtc_peerconnection fuzzer. Let me know if you need a link to its source code (but I suspect that won't help you). It's described in more detail here: https://sites.google.com/a/google.com/rtc-platform/test-engineering/fuzz-tests/chrome-web-api-fuzzing. If you want to see the generated html test case, look at the unminimized test case in the report in #4.

Note: Line numbers in video_frame.cc have changed since the time the issue has been created. The line 383 where the log message is produced has moved down to line 425 [1], because code was added [2].

[1] https://cs.chromium.org/chromium/src/media/base/video_frame.cc?l=425
[2] https://codereview.chromium.org/2096813002/diff/20001/media/base/video_frame.cc

Adding miu@ for thoughts on this, because the logging was set to LOG(DFATAL) as per miu@s suggestion in a code review [1] a few months back.

It seems that the fuzzer test [2] manages to trigger a call to VideoFrame::WrapVideoFrame() [3] with an invalid config. In this particular case it appears to be invalid because the visible_rect is 0x0.

I understand that we would like to get log messages in release builds for cases where frames are dropped because of some unexpected condition. But an invalid configuration that is (validly) passed in by a fuzzer test does not really seem to qualify as "unexpected".

Not sure what a proper solution would be. Maybe we could try to move the log message up the stack to a point where we can distinguish between failures that are somewhat expected (as in the case of the fuzzer) and failures that are truly unexpected. Or we could try to catch requests with invalid configurations earlier, before they reach VideoFrame::WrapVideoFrame() and report back an error without logging?

[1] https://codereview.chromium.org/1476523005/#msg6
[2] https://cluster-fuzz.appspot.com/testcase?key=4504027371143168
[3] https://cs.chromium.org/chromium/src/media/base/video_frame.cc?l=406

Oh, yeah, the fuzzer can do pretty much anything as it's trying to break the code. I think printing error messages is fine, the problem is just you're printing so many of them. Maybe you can rate-limit them a bit? Your other suggestions also SGTM.

1. Why are we fuzzing this function? Or, if it is being fuzzed indirectly, perhaps the fuzzer has revealed a bug in the calling function (since the caller should not have tried to create a VideoFrame using invalid arguments)?

2. The code inside this function seems to have left an open question: Is it valid to call this function with illegal arguments? Is that a crashable offense (i.e., client code should never attempt it)? Or, does all client code accept that nullptr is always a possible return value?

3. In the code review comment from a few months back, I was generalizing about all the DCHECKs() that were changed to LOG(ERROR) throughout the CL. IMHO, in this function, we really should be strictly CHECK() failing, since it's wrong for client code to request we wrap a frame that itself is invalid.

4. This function is *very dangerous* because it copies pointers to data held by wrapped VideoFrame, but does not hold a ref-counted reference to the wrapped frame. Thus, there's absolutely no code structure in place for guaranteeing the wrapped frame will not be destroyed while the wrapper is in use.

Re 2., I believe that all Wrap* code might return nullptr and client's are responsible for handling it.

Re 4. most clients add a DestructionObserver that holds a ref to original frame until new frame goes out of scope. It would be a good idea to just move adding this destruction observer inside WrapVideoFrame().
https://cs.chromium.org/chromium/src/media/base/video_util.cc?rcl=1469027825&l=367

Re #11:

1. Indirectly, we basically just set up weird WebRTC calls. It could well be a bug in the calling function.
2. Your call.
3. Sounds reasonable.
4. Right.

So you mean the answer here is to fail earlier in the call chain and not call video_frame.cc with weird arguments? That does sound reasonable. Emircan, can you do that?... I don't know if you're working in this particular code path. CC:ing some other random video people.

I will take care of it.

Referring to the items 1-4 discussed in comments #11, #12, and #13:

I believe there are two separate issues at play here:

Issue 1 (relates to Items 1-3): It is unclear if it is legal to wrap a frame with an invalid config.
Currently, the code suggests it is illegal, but we do not want to crash in release builds, 
and ask calling clients to provide a code path for us returning a nullptr.
The fuzzer managed to get an illegal invocation happening, which indicates a bug. 
The expected behavior would be to reject the illegal configuration earlier up the stack.
I will look into this and use the present issue entry to track the progress on it.

 Issue 2  (relates to Item 4): When wrapping an instance of VideoFrame in another VideoFrame, 
the wrapper does not automatically participate in (shared) ownership of the wrapped instance.
Instead, ownership is attached externally through "AddDestructionObserver()" and an empty 
method is used in combination with Base::Bind to hold a reference to the instance inside a 
Base::Closure. This solution seems far from clean.
A clean solution would be to have VideoFrame be a pure interface and then use the 
decorator pattern to provide different implementations for "regular" instances that 
hold data and "wrapping" (decorator) instances that mostly delegate to a wrapped instance
but allow for some modifications.
I have create a new issue entry [1] for this.

[1] https://bugs.chromium.org/p/chromium/issues/detail?id=631231

I have analyzed the first issue and here is what I found.

We get different behavior for the following example constraints passed into getUserMedia() from a JavaScript example page.

1.) video: { advanced: [{width: {max: 0 }}] }
2.) video: { width: { max: 0 } }
3.) video: { width: { ideal: 0 } }

Case 1.) is essentially what the Fuzz test did. It leads to the call to VideoFrame::WrapVideoFrame() [1] with illegal configuration (width = 0). This is happening, because the constraint is not rejected and a VideoFrameResolutionAdapter [2] is created that attempts to crop incoming frames to a width of 0. 

For case 2.) we get a ConstraintNotSatisfiedError reported back to JavaScript, which is probably what we want.

For case 3.) we get a video stream with a default resolution and the constraint is being ignored. That also seems reasonable.

The different behavior between the three cases is due to the handling of the constraints in MediaStreamVideoSource::FinalizeAddTrack() [3]. Case 3.) is ignored because ideal values are not (yet?) taken into account at all. Case 1.) is not rejected, because it is not a mandatory constraint, but then the constraint is used as parameter of the VideoFrameResolutionAdapter without checking whether or not it is valid.

Here is my recommendation:
1.) We should update the logic that selects a video format based on the given constraints in [3] to match the behavior described in the spec [4].
2.) If possible, we should cause a crash in Fuzz tests when VideoFrame::WrapVideoFrame() [1] is invoked with an illegal config. This way we can more clearly express that this indicates issues we want to look into.

@phoglund: Do you know a way to cause a crash (or other hard error) in Fuzz tests but not in production?

[1] https://cs.chromium.org/chromium/src/media/base/video_frame.h?q=WrapVideoFrame&dr=CSs&l=250
[2] https://cs.chromium.org/chromium/src/content/renderer/media/video_track_adapter.cc?q=VideoFrameResolutionAdapter&sq=package:chromium&l=55&dr=CSs
[3] https://cs.chromium.org/chromium/src/content/renderer/media/media_stream_video_source.cc?dr=CSs&q=FinalizeAddTrack&sq=package:chromium&l=492
[4] https://www.w3.org/TR/mediacapture-streams/#constrainable-interface

Re #16: your suggestions SGTM. That's nice, thorough work :)

No, the fuzzer runs on a regular (though ASAN-built) chromium. You mean you would want that crash to catch when WebRTC makes bad calls into video_frame? AFAIK there's no way to do that. We should not crash in production since any website can pass in the poisonous constraints and cause a crash, so I think you'll have to return an error up the chain.

I just checked on the latest code and found that the issue is still there.
Doing getUserMedia with constraints

  video: {
      advanced: [{width: {max: 0 }}]
  }

causes the tab to crash as explained in comment #16 case 1.).
Since code lines have shifted, here are updated references to the code:

* A track with the zero-width constraint is added in MediaStreamVideoSource::AddTrack() [1].
* The zero width is read from the track constraints in MediaStreamVideoSource::FinalizeAddTrack() [2].
* VideoFrame::WrapVideoFrame() with the illegal zero width is called in VideoFrameResolutionAdapter::DeliverFrame() [3].

I wonder what would be the right place to check for and reject optional 
constraint that cannot be satisfied. 

guidou@: You probably have the best understanding of this right now, since you 
have been working on the constraints. Will this maybe be resolved automatically 
as part of  issue 657733 ?

[1] https://cs.chromium.org/chromium/src/content/renderer/media/media_stream_video_source.cc?sq=package:chromium&dr=CSs&l=342
[2] https://cs.chromium.org/chromium/src/content/renderer/media/media_stream_video_source.cc?sq=package:chromium&dr=CSs&l=570
[3] https://cs.chromium.org/chromium/src/content/renderer/media/video_track_adapter.cc?sq=package:chromium&dr=CSs&l=269

chfremer@: sorry for the late reply. I think it should be resolved as part of  issue 657733 . Can you try with 59 or 60?

The specific case of 

  video: {  advanced: [{width: {max: 0 }}] }

should result in the advanced constraint being ignored since max width 0 cannot be satisfied.

Tested in a build from tip of tree today and confirmed that the case no longer causes the tab to crash. 

#20: Looks like it is working. Thanks!