VOOZH about

URL: https://phabricator.services.mozilla.com/D77893

⇱ ⚙ D77893 Bug 1623971 - P17: Set media-session's MediaImage to the SMTC interface

Paths

Bug 1623971 - P17: Set media-session's MediaImage to the SMTC interface
ClosedPublic

Authored by chunmin on Jun 2 2020, 7:13 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Jun 14, 8:27 PM
Unknown Object (File)
Fri, Jun 12, 6:28 AM
Unknown Object (File)
Mon, May 25, 10:51 PM
Unknown Object (File)
Mon, May 25, 4:46 AM
Unknown Object (File)
May 18 2026, 6:35 AM
Unknown Object (File)
May 17 2026, 8:57 PM
Unknown Object (File)
May 3 2026, 10:10 PM
Unknown Object (File)
Apr 6 2026, 4:22 PM

Details

Summary

This patch does the following things:

  1. Use to fetch the MediaImage defined in media-session
  2. Upon the above image is fetched, set it to the SMTC's thumbnail

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
phab-bot changed the visibility from "Custom Policy" to "Public (No Login Required)".
phab-bot changed the edit policy from "Custom Policy" to "Restricted Project (Project)".
phab-bot removed a project: secure-revision.
Comment Actions

Code analysis found 4 defects in the diff 289399:

  • 1 defect found by code coverage analysis
  • 3 defects found by clang-format

You can run this analysis locally with:

  • (C/C++)

For your convenience, here is a patch that fixes all the clang-format defects (use it in your repository with or ).

The analysis task source-test-mozlint-mingw-cap failed, but we could not detect any issue.
Please check this task manually.

In our previous code coverage analysis run, we found some files which had no coverage and are being modified in this patch:
Should they have tests, or are they dead code ?

  • You can file a bug blocking Bug 1415824 for untested files that should be tested.
  • You can file a bug blocking Bug 1415819 for untested files that should be removed.

If you see a problem in this automated review, please report it here.

You can view these defects on the code-review frontend and on Treeherder.

chunmin edited the summary of this revision. (Show Details)
Comment Actions

Code analysis found 1 defect in the diff 289439:

  • 1 defect found by code coverage analysis

In our previous code coverage analysis run, we found some files which had no coverage and are being modified in this patch:
Should they have tests, or are they dead code ?

  • You can file a bug blocking Bug 1415824 for untested files that should be tested.
  • You can file a bug blocking Bug 1415819 for untested files that should be removed.

If you see a problem in this automated review, please report it here.

You can view these defects on the code-review frontend and on Treeherder.

Comment Actions

Code analysis found 1 defect in the diff 289641:

  • 1 defect found by code coverage analysis

In our previous code coverage analysis run, we found some files which had no coverage and are being modified in this patch:
Should they have tests, or are they dead code ?

  • You can file a bug blocking Bug 1415824 for untested files that should be tested.
  • You can file a bug blocking Bug 1415819 for untested files that should be removed.

If you see a problem in this automated review, please report it here.

You can view these defects on the code-review frontend and on Treeherder.

widget/windows/WindowsSMTCProvider.cpp
434

Spoil: Now we fails to fetch default image. And we should probably don't fetch it and just use it directly since it's a local file and it's trustworthy. However, we need to find the correct path for the default image. The current default image path isn't included in the official build. This should be fixed in bug 1643102.

Comment Actions

Code analysis found 1 defect in the diff 290304:

  • 1 defect found by code coverage analysis

In our previous code coverage analysis run, we found some files which had no coverage and are being modified in this patch:
Should they have tests, or are they dead code ?

  • You can file a bug blocking Bug 1415824 for untested files that should be tested.
  • You can file a bug blocking Bug 1415819 for untested files that should be removed.

If you see a problem in this automated review, please report it here.

You can view these defects on the code-review frontend and on Treeherder.

Comment Actions

Code analysis found 1 defect in the diff 290424:

  • 1 defect found by code coverage analysis

In our previous code coverage analysis run, we found some files which had no coverage and are being modified in this patch:
Should they have tests, or are they dead code ?

  • You can file a bug blocking Bug 1415824 for untested files that should be tested.
  • You can file a bug blocking Bug 1415819 for untested files that should be removed.

If you see a problem in this automated review, please report it here.

You can view these defects on the code-review frontend and on Treeherder.

Comment Actions

Per offline discussion with chun-min, we would make some change on this patch.

widget/windows/WindowsSMTCProvider.h
85

why we need this? I didn't see the implementation of this function.

alwu requested changes to this revision.Jun 4 2020, 7:49 PM
This revision now requires changes to proceed.Jun 4 2020, 7:49 PM
Comment Actions

please feel free to reset r+ if there are other significant changes

widget/windows/WindowsSMTCProvider.cpp
357–399

can we just return in this case?

441

if mImageFetcher != nullptr before this line, do we need to do any cleanup before replacing the old reference?

if not, then i an assert ==nullptr would be good.

widget/windows/WindowsSMTCProvider.cpp
357–399

Sure

441
Comment Actions

Code analysis found 1 defect in the diff 293689:

  • 1 defect found by code coverage analysis

In our previous code coverage analysis run, we found some files which had no coverage and are being modified in this patch:
Should they have tests, or are they dead code ?

  • You can file a bug blocking Bug 1415824 for untested files that should be tested.
  • You can file a bug blocking Bug 1415819 for untested files that should be removed.

If you see a problem in this automated review, please report it here.

You can view these defects on the code-review frontend and on Treeherder.

chunmin edited the summary of this revision. (Show Details)
Comment Actions

Code analysis found 2 defects in the diff 294388:

  • 1 defect found by clang-tidy
  • 1 defect found by code coverage analysis

You can run this analysis locally with:

  • (C/C++)

In our previous code coverage analysis run, we found some files which had no coverage and are being modified in this patch:
Should they have tests, or are they dead code ?

  • You can file a bug blocking Bug 1415824 for untested files that should be tested.
  • You can file a bug blocking Bug 1415819 for untested files that should be removed.

If you see a problem in this automated review, please report it here.

You can view these defects on the code-review frontend and on Treeherder.

dom/media/mediacontrol/FetchImageHelper.cpp
75

can be contructed from a raw pointer, so we can use directly, right?

https://searchfox.org/mozilla-central/source/xpcom/base/nsCOMPtr.h#510-517

dom/media/mediacontrol/FetchImageHelper.h
30–31

Add .

widget/windows/WindowsSMTCProvider.cpp
120

move this to .

358

I would prefer to rename this function in order to indicate that this function is not just about "getting an encoder", because we actually do the encoding as well in this function.

nit : maybe we can use something like to merge these two functions.

Or, . is a class where you can use to access its data buffer and use to access its buffer length.

403

If the incoming URL is as same as the previous one and we're still in the process of fetching, then calling this would result in aborting our fetching because of line#415-418.

445

because you've captured , no need to use . You can call directly.

widget/windows/WindowsSMTCProvider.h
106

is ref-countable, so you can't use unless we remove the refcounting support for .

After checking again, I think we can remove the support for refcounting and use instead.

chunmin marked 6 inline comments as done.
dom/media/mediacontrol/FetchImageHelper.cpp
75

Yeh

dom/media/mediacontrol/FetchImageHelper.h
30–31

Oh I forget to put it back.

widget/windows/WindowsSMTCProvider.cpp
358

Sounds good.

Return will need to do a copy. Instead, I'll merge and into

403

I'll move it to be the above of

445

Sure.

Comment Actions

Code analysis found 1 defect in the diff 295265:

  • 1 defect found by code coverage analysis

In our previous code coverage analysis run, we found some files which had no coverage and are being modified in this patch:
Should they have tests, or are they dead code ?

  • You can file a bug blocking Bug 1415824 for untested files that should be tested.
  • You can file a bug blocking Bug 1415819 for untested files that should be removed.

If you see a problem in this automated review, please report it here.

You can view these defects on the code-review frontend and on Treeherder.

widget/windows/WindowsSMTCProvider.cpp
417

You will need to disconnect in order not to show the previous fetching result.

widget/windows/WindowsSMTCProvider.h
108

nit : If we really need this comment, I will say something like "This is used to track the image fetch process". Or simply remove this comment.

Comment Actions

Code analysis found 1 defect in the diff 295315:

  • 1 defect found by code coverage analysis

In our previous code coverage analysis run, we found some files which had no coverage and are being modified in this patch:
Should they have tests, or are they dead code ?

  • You can file a bug blocking Bug 1415824 for untested files that should be tested.
  • You can file a bug blocking Bug 1415819 for untested files that should be removed.

If you see a problem in this automated review, please report it here.

You can view these defects on the code-review frontend and on Treeherder.

widget/windows/WindowsSMTCProvider.cpp
366

I found this function is wrong and may cause UAF. The image data would be in but it would be destroyed after the function call. I'll fix it later.

Comment Actions

Code analysis found 1 defect in the diff 295352:

  • 1 defect found by code coverage analysis

In our previous code coverage analysis run, we found some files which had no coverage and are being modified in this patch:
Should they have tests, or are they dead code ?

  • You can file a bug blocking Bug 1415824 for untested files that should be tested.
  • You can file a bug blocking Bug 1415819 for untested files that should be removed.

If you see a problem in this automated review, please report it here.

You can view these defects on the code-review frontend and on Treeherder.

thomasmo added inline comments.
widget/windows/WindowsSMTCProvider.cpp
366

wait-- if you know there's a UAF, you can't check that in.

is there some way to work around that? or introduce a release_assert for some preceding condition?

This revision now requires changes to proceed.Jun 12 2020, 2:33 AM
chunmin added inline comments.
widget/windows/WindowsSMTCProvider.cpp
366

Don't worry, it's fixed. The new code below transfers the ownership of the to so there is no UAF anymore.

thomasmo added inline comments.
widget/windows/WindowsSMTCProvider.cpp
366

ah, okay. thanks for confirming :-)

This revision now requires review to proceed.Jun 12 2020, 3:35 PM
chunmin marked 5 inline comments as done.
Comment Actions

Code analysis found 1 defect in the diff 295849:

  • 1 defect found by code coverage analysis

In our previous code coverage analysis run, we found some files which had no coverage and are being modified in this patch:
Should they have tests, or are they dead code ?

  • You can file a bug blocking Bug 1415824 for untested files that should be tested.
  • You can file a bug blocking Bug 1415819 for untested files that should be removed.

If you see a problem in this automated review, please report it here.

You can view these defects on the code-review frontend and on Treeherder.

Comment Actions

Remember to disconnect when there is no valid image url.

This revision is now accepted and ready to land.Jun 15 2020, 5:22 PM
Comment Actions

Code analysis found 1 defect in the diff 296761:

  • 1 defect found by code coverage analysis

In our previous code coverage analysis run, we found some files which had no coverage and are being modified in this patch:
Should they have tests, or are they dead code ?

  • You can file a bug blocking Bug 1415824 for untested files that should be tested.
  • You can file a bug blocking Bug 1415819 for untested files that should be removed.

If you see a problem in this automated review, please report it here.

You can view these defects on the code-review frontend and on Treeherder.

chunmin marked an inline comment as done.
chunmin added inline comments.
widget/windows/WindowsSMTCProvider.cpp
417

Yeah good catch

Comment Actions

Code analysis found 1 defect in the diff 296811:

  • 1 defect found by code coverage analysis

In our previous code coverage analysis run, we found some files which had no coverage and are being modified in this patch:
Should they have tests, or are they dead code ?

  • You can file a bug blocking Bug 1415824 for untested files that should be tested.
  • You can file a bug blocking Bug 1415819 for untested files that should be removed.

If you see a problem in this automated review, please report it here.

You can view these defects on the code-review frontend and on Treeherder.

Comment Actions

Code analysis found 1 defect in the diff 298946:

  • 1 defect found by code coverage analysis

In our previous code coverage analysis run, we found some files which had no coverage and are being modified in this patch:
Should they have tests, or are they dead code ?

  • You can file a bug blocking Bug 1415824 for untested files that should be tested.
  • You can file a bug blocking Bug 1415819 for untested files that should be removed.

If you see a problem in this automated review, please report it here.

You can view these defects on the code-review frontend and on Treeherder.

Revision Contents

PathSize
dom/
media/
mediacontrol/
9 lines
7 lines
widget/
windows/
9 lines
128 lines
CommitParentsAuthorSummaryDate
b57d9a939f73Chun-Min Chang
Bug 1623971 - P17: Set media-session's MediaImage to the SMTC interface r=alwu… (Show More…)
StatusAuthorRevision
Closedchunmin
Closedchunmin
Closedchunmin
Closedchunmin
Closedchunmin
Closedchunmin
Closedchunmin
Closedchunmin
Closedchunmin
Closedchunmin
Closedchunmin
Closedchunmin
Closedchunmin
Closedchunmin
Closedchunmin
Closedchunmin
Closedchunmin
Closedchunmin
Closedchunmin

Diff 298965

dom/media/mediacontrol/FetchImageHelper.h

Loading...

dom/media/mediacontrol/FetchImageHelper.cpp

Loading...

widget/windows/WindowsSMTCProvider.h

Loading...

widget/windows/WindowsSMTCProvider.cpp

Loading...