1531708 - nsSyncStreamListener drops error status when waiting for data

Status: RESOLVED FIXED

(Core :: Networking, enhancement, P2)

Description

[[e-mail]]

This bug seems to have been present in nsSyncStreamListener since it was originally added by bug 192284.

https://searchfox.org/mozilla-central/source/netwerk/base/nsSyncStreamListener.cpp#117

mStatus is set as the result of an OnStopRequest call. If this happens before the call to Available, then the status is returned as expected. If however Available starts waiting for data, it overwrites the status with the return value from WaitForData, which is usually NS_OK.

My guess as to the easiest way to fix this is to make WaitForData return mStatus on success.

Comment 1

[Valentin Gosu [:valentin] (he/him)]

Junior, could you take a look at this?

Comment 2

[Junior [inactive]]

Hello neil,
Sorry for missing this.

Did you worry about this sequence?
listener->OnStopRequest() -> mStatus is set by parameter
then listener->Available() -> mStatus is set to NS_OK

However IMO mStatus won't change by Available in this case since we bailed out for failure
https://searchfox.org/mozilla-central/rev/201450283cddc9e409cec707acb65ba6cf6037b1/netwerk/base/nsSyncStreamListener.cpp#116

And nSyncStreamListener is single thread object. No race condition happened.

Comment 3

[[e-mail]]

(In reply to Junior from comment #2)

Hello neil,
Sorry for missing this.

Sorry for overlooking your response!

Did you worry about this sequence?
listener->OnStopRequest() -> mStatus is set by parameter
then listener->Available() -> mStatus is set to NS_OK

However IMO mStatus won't change by Available in this case since we bailed out for failure
https://searchfox.org/mozilla-central/rev/201450283cddc9e409cec707acb65ba6cf6037b1/netwerk/base/nsSyncStreamListener.cpp#116

And nSyncStreamListener is single thread object. No race condition happened.

WaitForData() calls SpinEventLoopUntil, which means that an event can call listener->OnStopRequest() after the check for failure but before listener->Available() sets mStatus back to NS_OK.

Comment 4

[Junior [inactive]]

Attachment: Bug 1531708 - avoid hiding error in nsSyncStreamListener

Comment 5

[Junior [inactive]]

Let's see if treeherder is happy
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4beed9b125aa5d4072f5d7844ef84f45ce66f499
https://treeherder.mozilla.org/#/jobs?repo=try&revision=275062eaa02ea1a178ef3a651ad9572ca0f5503c

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6b3d6a63e8c4
avoid hiding error in nsSyncStreamListener r=michal

Comment 7

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/6b3d6a63e8c4