| Summary: | Make RealtimeOutgoingVideoSource use DestructionThread::Main | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | youenn fablet <youennf> | ||||
| Component: | WebRTC | Assignee: | youenn fablet <youennf> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | achristensen, cdumez, commit-queue, eric.carlson, webkit-bug-importer, youennf | ||||
| Priority: | P2 | Keywords: | InRadar | ||||
| Version: | WebKit Nightly Build | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Attachments: |
|
||||||
|
Description
youenn fablet
2018-07-06 12:45:39 PDT
Created attachment 344442 [details]
Patch
Comment on attachment 344442 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=344442&action=review > Source/WebCore/platform/mediastream/RealtimeOutgoingVideoSource.h:57 > + auto result = hasOneRef() ? rtc::RefCountReleaseStatus::kDroppedLastRef : rtc::RefCountReleaseStatus::kOtherRefsRemained; Is this thread safe? If Release() can be called from various threads, then a ref() / deref() can happen on another thread after your hasOneRef() check. > View in context:
> https://bugs.webkit.org/attachment.cgi?id=344442&action=review
>
> > Source/WebCore/platform/mediastream/RealtimeOutgoingVideoSource.h:57
> > + auto result = hasOneRef() ? rtc::RefCountReleaseStatus::kDroppedLastRef : rtc::RefCountReleaseStatus::kOtherRefsRemained;
>
> Is this thread safe? If Release() can be called from various threads, then a
> ref() / deref() can happen on another thread after your hasOneRef() check.
Well, this whole API is not really thread safe but we need to return something as an API contract. Even if we return the true value at the time we did defer(), it will be useless for the caller, except maybe for logging.
I am fine continuing returning rtc::RefCountReleaseStatus::kOtherRefsRemained like done before.
Comment on attachment 344442 [details] Patch Clearing flags on attachment: 344442 Committed r233602: <https://trac.webkit.org/changeset/233602> All reviewed patches have been landed. Closing bug. |