mkhahani@ Could you please help us with sample extension and steps to reproduce the issue, so that we can test it from TE-End

Thank You...

I attached the extension. Tested also on the latest version of Chromium and the same result.

Thanks.

Thank you for your response.
Unable to load the provided extension.
Attaching the screenshot for your reference. 
mkhahani@ could you please look into it and let us know your observations.

Deleted: Issue 675521.PNG 48.6 KB

	Issue 675521.PNG 48.6 KB View Download

I don't understand. Don't you have the manifest file after extracting the extension? I downloaded the attachment and I could add it as unpacked extension. I attached the manifest.json again. Let me know if it works.

Deleted: manifest.json 1.2 KB

manifest.json 1.2 KB View Download

Confirmed on Linux. This looks like a memory safety issue (heap-use-after-free), restricting view to security team out of caution. Although the bug is reproduced with an extension, it is not impossible that the underlying issue can be triggered from the web.

Steps to reproduce:
1. Download attached zip file, extract it to a separate directory.
2. Visit chrome://extensions and load the extension.
3. Visit any website, e.g. example.com and click on the extension button.
4. Patiently wait (I went off to do something else for a few minutes, and upon focusing Chrome again the extension crashed).

See attached ASan log, here is the head of the trace:

==72==ERROR: AddressSanitizer: heap-use-after-free on address 0x60b000093cd8 at pc 0x5651c862e814 bp 0x7fff9470a450 sp 0x7fff9470a448
READ of size 8 at 0x60b000093cd8 thread T0 (chrome)
    #0 0x5651c862e813 in timestamp ./out/asan/../../third_party/libwebm/source/mkvmuxer/mkvmuxer.h:123:39
    #1 0x5651c862e813 in WriteFramesLessThan ./out/asan/../../third_party/libwebm/source/mkvmuxer/mkvmuxer.cc:3843:0
    #2 0x5651c862d516 in MakeNewCluster ./out/asan/../../third_party/libwebm/source/mkvmuxer/mkvmuxer.cc:3574:8
    #3 0x5651c862b783 in DoNewClusterProcessing ./out/asan/../../third_party/libwebm/source/mkvmuxer/mkvmuxer.cc:3640:24
    #4 0x5651c862a3b7 in AddGenericFrame ./out/asan/../../third_party/libwebm/source/mkvmuxer/mkvmuxer.cc:3257:8
    #5 0x5651c8629fbd in AddFrame ./out/asan/../../third_party/libwebm/source/mkvmuxer/mkvmuxer.cc:3173:10
    #6 0x5651c7d10d22 in AddFrame ./out/asan/../../media/muxers/webm_muxer.cc:319:12
    #7 0x5651c7d10d22 in OnEncodedVideo ./out/asan/../../media/muxers/webm_muxer.cc:163:0
    #8 0x5651c69ec193 in OnEncodedVideo ./out/asan/../../content/renderer/media/media_recorder_handler.cc:269:16

Deleted: crbug675521.zip 59.5 KB

crbug675521.zip 59.5 KB Download

Deleted: crbug675521-asan-chromium-55.0.2883.75.log 17.8 KB

crbug675521-asan-chromium-55.0.2883.75.log 17.8 KB View Download

mcasas, I can repro this. Can you check if it's a recording but or if it should be duped by any of the other bugs with same signature?

I could repro a similar signature, however I wonder if it would
still repro after emircan@ landed  https://crbug.com/678095  a 
minute ago?

 https://crbug.com/678095  is Fixed, can you still repro the problem?

I kept Chromium	57.0.2975.0 open for an hour (following the steps from comment 7, with step 3 = click on the record icon in the popup that opens when the extension button is clicked) and the extension process did not crash. So I guess that the issue has been fixed.

Marking as Fixed per #13, and removing the Restrict-View-...,

mkhahani@gmail.com, rob@robwu.nl, please reopen this bug if
it the problem resurfaces.  Thanks to emircan@ that solved
 https://crbug.com/678095  !

Still crashes after minutes tested on Chromium version 57.0.2975.0 64-bit :(
Try recording multiple tabs simultaneously.

I'm reproducing a crash when recording two tabs (https://blog.chromium.org/),
after a while I'm getting the following crash stack:


#0 0x7f631b0c364e base::debug::StackTrace::StackTrace()
#1 0x7f631b0c318f base::debug::(anonymous namespace)::StackDumpSignalHandler()
#2 0x7f631b51c330 <unknown>
#3 0x7f631250b75c mkvmuxer::Frame::discard_padding()
#4 0x7f6312508951 mkvmuxer::Segment::WriteFramesAll()
#5 0x7f631250a2be mkvmuxer::Segment::DoNewClusterProcessing()
#6 0x7f6312509827 mkvmuxer::Segment::AddGenericFrame()
#7 0x7f63125095f4 mkvmuxer::Segment::AddFrame()
#8 0x7f6311fd3d12 media::WebmMuxer::AddFrame()
#9 0x7f6311fd30b6 media::WebmMuxer::OnEncodedVideo()
...

An interesting circumstance is that I see logs of Audio being constantly
recorded but video is not, encoded video frame bursts show up when I 
scroll in either of the recorded tabs.  Because of that I think this issue 
might be an impersonation of [1] and it was already found by DrMemory
some time ago in  Issue 571551 .


[1] https://monorail-prod.appspot.com/p/webm/issues/detail?id=1045

It could be that [1] is the same root cause, from there:

> 2. Add only audio frames, making the cluster have a duration longer than the max cluster duration.
> 3. Add a new video keyframe.
> 4. Finalize de segment.
--> Crash

According to [2]:
> ... Allows every cluster to have blocks with positive values up to 32.767 seconds.

Which coincides with what I'm reproducing, steps:

1. Open e.g. https://blog.chromium.org/ 
2. Start recording
2b. The recorded site cannot have animations or any other active content to prevent 
video frames from being generated
3. don't move/scroll for > 32 seconds. 
3b. The flag -vmodule='*webm_muxer*=2' can be used on developer builds to see 
guiding logs such as e.g.
[1:1:0109/122359.661172:VERBOSE2:webm_muxer.cc(178)] OnEncodedAudio - 8B 45.5998 s
[1:1:0109/122359.691428:VERBOSE1:webm_muxer.cc(139)] OnEncodedVideo - 53350B 45.322 s
4. Stop the recording
5. Crash:

#12 0x7fdac85c7327 ShimCppDelete
#13 0x7fdabf74d1be mkvmuxer::Frame::~Frame()
#14 0x7fdabf755cb5 mkvmuxer::Segment::~Segment()
#15 0x7fdabf220b1a media::WebmMuxer::~WebmMuxer()
#16 0x7fdabf220b99 media::WebmMuxer::~WebmMuxer()
#17 0x7fdac1ba02ef std::default_delete<>::operator()()
#18 0x7fdac2db6fcc std::unique_ptr<>::reset()
#19 0x7fdac2db608c content::MediaRecorderHandler::stop()
#20 0x7fdaad3ea2d4 blink::MediaRecorder::stopRecording()
#21 0x7fdaad3ea1a4 blink::MediaRecorder::stop()


[1] https://monorail-prod.appspot.com/p/webm/issues/detail?id=1045
[2] https://www.webmproject.org/docs/container/#muxer-guidelines

The cause of the problem here appears to be that WebmMuxer is ignoring return values from libwebm:

https://cs.chromium.org/chromium/src/media/muxers/webm_muxer.cc?rcl=0&l=323

That method returns bool. True means success. False means error. My simple test case shows Segment::AddFrame() returning false when I attempt reproducing this problem in a simple test case. A quick look at the link above shows that the return value from the library is not captured. There's not a lot we can do upstream when the library crashes because return values aren't being inspected.

tomfinegan@ traced the problem to being an as-yet-unseen case of starving
libwebm muxer of video while in presence of audio and has come up with a 
solution.  Once libwebm lands that patch, we'll roll libwebm in Cr against
this bug. (patch in attachment).

In the meantime I'll handle those unhandled return values :)

Deleted: libwebm.patch 1.2 KB

libwebm.patch 1.2 KB Download

The following revision refers to this bug:
  https://chromium.googlesource.com/webm/libwebm/+/4956b2dec65352af32dc71bab553acb631c64177

commit 4956b2dec65352af32dc71bab553acb631c64177
Author: Tom Finegan <tomfinegan@google.com>
Date: Thu Jan 12 21:15:46 2017

mkvmuxer: Force new clusters when audio queue gets too long.

Force creation of a new Cluster when writing queued audio would
cause an error due to violating maximum block duration.

BUG= 675521 

Change-Id: I6ad09c2a2f71d95bb04eed5ead04dc8072aaa59d

[modify] https://crrev.com/4956b2dec65352af32dc71bab553acb631c64177/mkvmuxer/mkvmuxer.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ed7c87b8816f37e73f640e3b7c5c93fad0dbd29a

commit ed7c87b8816f37e73f640e3b7c5c93fad0dbd29a
Author: mcasas <mcasas@chromium.org>
Date: Sat Jan 14 03:39:46 2017

MediaRecorder: handle libwebm::Segment::AddFrame() errors and wire up to Blink

libwebm::Segment::AddFrame() can and will return false on error
conditions, but the return value is currently, sadly, ignored.
This CL handles the return value and wires it to Blink's
onError() callback.

Unittests are beefed up, in particular:

- Added WebmMuxer::ForceOneLibWebmErrorForTesting(), and using it
in WebmMuxerTest to force errors and then
EXPECT_FALSE(...OnEncodedVideo/OnEncodedAudio)

- Thanks to scheib@ insistence in testing, I found an unreachable
code and a bug (!) in WebmMuxer:

-- the dumping of |encoded_frames_queue_| [1] is not reachable
because the said deque is emptied in OnEncodedAudio() (left a
DCHECK() instead).

-- the bug, introduced in this very CL, was the use-after-free
during emptying the same deque, when retrying emptying it after an
error.

- MediaRecorderHandlerTest adds a test case to verify that a
muxer error causes an onError() to be bubbled up to Blink.

All in all, testing FTW!

[1] https://cs.chromium.org/chromium/src/media/muxers/webm_muxer.cc?q=webm_muxer.cc&sq=package:chromium&dr&l=162

BUG= 675521 

Review-Url: https://codereview.chromium.org/2633623002
Cr-Commit-Position: refs/heads/master@{#443780}

[modify] https://crrev.com/ed7c87b8816f37e73f640e3b7c5c93fad0dbd29a/content/renderer/media/media_recorder_handler.cc
[modify] https://crrev.com/ed7c87b8816f37e73f640e3b7c5c93fad0dbd29a/content/renderer/media/media_recorder_handler_unittest.cc
[modify] https://crrev.com/ed7c87b8816f37e73f640e3b7c5c93fad0dbd29a/media/muxers/webm_muxer.cc
[modify] https://crrev.com/ed7c87b8816f37e73f640e3b7c5c93fad0dbd29a/media/muxers/webm_muxer.h
[modify] https://crrev.com/ed7c87b8816f37e73f640e3b7c5c93fad0dbd29a/media/muxers/webm_muxer_unittest.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8853f146f7947858543432dbe4dc4329dd72e83e

commit 8853f146f7947858543432dbe4dc4329dd72e83e
Author: mcasas <mcasas@chromium.org>
Date: Mon Jan 16 19:32:06 2017

libwebm: roll 9a235e0:4956b2d

Note: libwebm is only used for MediaRecorder.

BUG= 675521 

.
$ git log --pretty=oneline 9a235e0..4956b2d --abbrev-commit
4956b2d mkvmuxer: Force new clusters when audio queue gets too long.
54f1559 cmake: Cache results of CXX flag tests.
81c73fc mkvparser: Avoid alloc failures in SeekHead::Parse.
9732ae9 EbmlElementSize: quiet uint64->int32 conv warning
da04eba SetProjectionPrivate: quiet uint64->size_t conv warning
5e1d131 Merge "mkvparser,Projection::Parse: fix int->bool conv"
6db32d5 mkvparser,Projection::Parse: fix int->bool conv
3bb0dfa cosmetics: fix a couple lint warnings
0e179d6 update .clang-format
fc5f88d Fix temp files being left on system.
c04a134 Add support for overriding PixelWidth and PixelHeight.
c0160e0 Add support to explicitly set segment duration.
02bc809 Add support to estimate file duration.
c97e3e7 Add support to output sub-sample encryption information.
26f4344 MakeUID: quiet unused param warning in Android builds
d6af52a Change check to fix compile error.
1720020 webm_parser: Add Mesh value for ProjectionType
78f2c5a webm_parser: Use ./ prefix for includes
da62f65 webm_parser: Remove webm/ prefix from public includes
e15e8f2 webm_parser: Update README build instructions
5023f2b mkvmuxer: Fix Colour::Valid()
cf16204 mkvmuxer_tests: Actually test cue points in the cue point test.
93e9fb3 Validate Colour element values.
8036925 mkvparser_tests: Add Projection element test.
f52d38c mkvparser_tests: Add Colour element test.
826436a mkvparser: minor SeekHead::Entry clean up.
24fb44a mkvmuxer_tests: Add Projection element test.
1e0a8ea mkvmuxer_tests: Add Colour element test.
0278616 mkvmuxer: Colour accessors/mutators.
ce52f6e Merge "Add mkvparser wrapper functions."
6e5cbc0 Merge "webm_info: Add Projection element support."
0a039cb Merge "mkvmuxer_sample: Add support for Projection element."
149e8e9 Merge "mkvparser_sample: Add support for Projection element."
43f40c4 Merge "mkvparser: Add Projection element support."
0c9a19e Merge "mkvmuxer: Add Projection element support."
ff667d5 Merge "Add support for the Projection element"
2346f8f Add mkvparser wrapper functions.
54d6b6b webm_info: Add Projection element support.
65fee06 mkvmuxer_sample: Add support for Projection element.
9a3f2b5 mkvparser_sample: Add support for Projection element.
41e814a mkvparser: Add Projection element support.
483a0ff mkvmuxer: Add Projection element support.
676a713 Add support for the Projection element
725f362 mkvmuxer: Fix memory leak when Colour is set multiple times.
fa182de mkvparser_sample: Add output of audio track codec private size.
4aa5338 Merge "mkvparser_tests: Add invalid BlockGroup test."
8f521f2 mkvparser_tests: Add invalid BlockGroup test.
dae3d48 Merge "Remove docs saying binary elements default to 0"
39137d7 Remove docs saying binary elements default to 0
2349904 Merge "Fix legacy Makefile."
16e5d2a Merge "Do not skip over unknown elements at the root level"
80685d3 Do not skip over unknown elements at the root level
c147504 Fix legacy Makefile.
58711e8 mkvparser_sample: Fix version info string.
837746f mkvparser_tests: Add invalid block test.
207cd80 Disambiguate sample sources and targets.
a112d71 mkvparser_tests: Refactor invalid file loading code.
5dea33e Disambiguate test source and target names.
125049e parser_tests: Add another truncated chapter string test.
1de8d4c parser_tests: Add truncated chapter string test.
ff8c2b6 parser_tests: Move cue validation to test_util.
4b0690f parser_tests: Add invalid lacing test.
64ae831 Merge "mkvmuxer: Set default doc type version to 4."
a351196 Merge "webm_parser: Reference more files in CMakeLists.txt."
9828e39 mkvmuxer: Set default doc type version to 4.
5495a59 webm_parser: Reference more files in CMakeLists.txt.
0c0ecd0 vpxpes_parser: Add start code emulation prevention support.
639a4bc webm2pes: Remove debug printfs().
9a51102 webm2pes: fflush() in the correct conversion function.
dc7f155 webm2pes: Track total bytes written.
d518128 webm_parser: Enable usage of werror.
e1fe762 webm2pes: Add test for mux/demux of large input.
1b24a79 vpxpes_parser: Read and store PTS when present.
6cf0a0f vpxpes_parser: Store frame payloads.
25d2602 webm_parser: Convert style to match the rest of libwebm
24be76d webm2pes: Replace VpxFrame with VideoFrame.
b451c3b Add a basic video frame storage class.
05c90eb libwebm_util: Clarify error text in superframe parser.
e6415af webm2pes: Make WritePesPacket() a public method.
8f840dd webm2pes: Move frame read out of PES packet write method.
448af97 webm2pes: Restore frame fragmentation support.
f8bb714 cmake: Integrate new parsing API and tests.
cb8ce0b Add a new incremental parsing API
900d322 vpxpes_parser/webm2pes: BCMV and PTS fixes.
4b73545 webm2pes: Add start code emulation prevention.
82903f3 Add column tiles and frame parallel to webm_info
5d91edf style_clean_up: Remove unnecessary parentheses
a95aa4b vp9_level_stats: correct total_uncompressed_bits_ calculation
f46566f mkvreader: Fix shorten-64-to-32 warning in 32 bit builds.
76630ca mkvwriter: Fix shorten-64-to-32 warning in 32 bit builds.
a8ffbd4 webm2pes: Fix format specifier warnings.
2101548 Merge "Add MaxLumaSampleRate grace percent to stats."
faf89d4 Add MaxLumaSampleRate grace percent to stats.
d31e6c9 Fix profile 2 in vp9_header_parser.
4fc66da Merge "Add flag to estimate last frame's duration to stats."
bd3ab3a Add flag to estimate last frame's duration to stats.
c89b968 Merge "Fix lint issue in hdr_util.h"
db5693d Merge "Add test for Cluster memory leak"
c182ed9 Fix lint issue in hdr_util.h
cc62ecd Add test for Cluster memory leak
196708a Change MaxLumaSampleRate to be based on frame resolution.
26d673e Merge "mkvmuxer: Fix leak when a Cluster isn't finalized"
eacb314 Merge "Add parsing support for new features in CodecPrivate."
cbd676b mkvmuxer: Fix leak when a Cluster isn't finalized
47f2843 Add parsing support for new features in CodecPrivate.

Review-Url: https://codereview.chromium.org/2631893003
Cr-Commit-Position: refs/heads/master@{#443933}

[modify] https://crrev.com/8853f146f7947858543432dbe4dc4329dd72e83e/DEPS

mkhahani@, robwu@, #24 and #25 should have addressed this issue,
can you please verify with a recent Canary? Thx.

Marking the issue as Fixed, tentatively.

Yeah it seems has been fixed!
Thank you all, guys.

Can we expect this fix in earlier versions(56 or 55)?

This bug requires manual review: We are only 10 days from stable.
Please contact the milestone owner if you have questions.
Owners: amineer@(clank), cmasso@(bling), gkihumba@(cros), bustamante@(desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

#32: Since this change needs at least one CL and a big roll,
I don't thin we'll have enough time to bake it properly in 
the non-stable channels, so let's not merge it back.