Details
- Reviewers
emilio padenot robwu nalexander kershaw - Group Reviewers
necko-reviewers geckoview-reviewers extension-reviewers media-playback-reviewers firefox-style-system-reviewers - Commits
- rFIREFOXAUTOLAND524d0b5cd55c: Bug 2036929 - Hold strong ref while calling nsIStreamListener methods r=necko…
- Bugzilla Bug ID
- 2036929
Diff Detail
- Repository
- rFIREFOXAUTOLAND firefox-autoland
Event Timeline
FWIW don't we already have a pre-existing clang-plugin check that we could use for this? This seems basically like it wants MOZ_CAN_RUN_SCRIPT...
| netwerk/base/nsStreamListenerTee.cpp | ||
|---|---|---|
| 56–61 | Q: should we also drop the aliased listener here? | |
ChannelWrapper changes look good, but the introduction of the same pattern to StreamFilterParent looks questionable (not unsafe, but confusing). Could you check my comment and share your thoughts on that?
| toolkit/components/extensions/webrequest/StreamFilterParent.cpp | ||
|---|---|---|
| 465 | Adding a nsCOMPtr to capture mOrigListener is not wrong, but it does signal uncertainty about the lifetime of mOrigListener and this (StreamFilterParent that already holds a nsCOMPtr to the listener). ODA here is invoked on StreamFilterParent::mOrigListenerand being passed StreamFilterParent::mChannel. I traced all places that you changed in this patch, and in all cases, this (StreamFilterParent) is already held by a strong reference. Which also implies that mOrigListener is captured by a strong reference. Beyond this line, mOffset below is touched. So if we could not rely on this being captured, then we would have an unsafe access to mOffset below. I'm not sure if applying a blanked nsCOMPtr here adds much value, on the contrary the expressed uncertainty on the lifetime of this may be more confusing. WDYT? | |
| netwerk/base/nsStreamListenerTee.cpp | ||
|---|---|---|
| 56–61 | It would get dropped at the end of the method anyway, but we can do it to preserve release order compared to mObserver | |
| toolkit/components/extensions/webrequest/StreamFilterParent.cpp | ||
| 465 | I agree that mOrigListener is probably safe as it only gets freed in the destructor, but we need the extra ComPtr if we want to enforce this via static analyzer / clang-plugin in the future. 🤔 | |
The change to StreamFilterParent is not ideal, but that is more a readability/maintainability concern than a correctness one, so I'm signing off to unblock the patch from the extension-reviewers perspective.
Side note: With the introduction of nsCOMPtr / RefPtr all over the place, I wondered whether all uses of refcounting were thread-safe (or at least only called on a single thread). I asked Claude to confirm that, and it affirmed that the usage is safe.
| toolkit/components/extensions/webrequest/StreamFilterParent.cpp | ||
|---|---|---|
| 465 | Is it possible to make the static analyzer happy without the addition here? E.g. that it detects that this is already captured, or an annotation of some sort. While I welcome the bugfix in the other part of the codebase, I find it unfortunate that the introduced pattern here signals uncertainty about the surrounding code that is not the case. Logically the combination "mOrigListener needs to be captured in a nsCOMPtr for safety" + "mOrigListener is only freed from the destructor" implies "other members here may be unsafe". If you find a way to avoid "unnecessary" application of the pattern, that would be welcome, but I'm not going to block the patch from landing. | |
r+ for mobile/android: if this is acceptable elsewhere, it's fine here. It hurts that all of these callers have to silently know this is required, but hopefully the lint solves that problem for future us...