242313 - nsIStreamListener does not document that onStartRequest/onStopRequest must be called

Status: RESOLVED FIXED

(Core :: Networking, defect, P5)

Description

[timeless]

should something in nsIStreamListener's documentation mention that you have to
call start/end?
<bz> that's standard fare for nsIStreamListener, no?
> nsIStreamListener doesn't mention it
> all nsIStreamListener has is oDA
<bz> er...
> start/stop are on its parent (nsIRequestObserver)
<bz> ah
> http://lxr.mozilla.org/seamonkey/source/netwerk/base/public/nsIStreamListener.idl
<bz> and nsIRequestObserver sayd?
<bz> er, says?
> which i consider to be fairly evil
> http://lxr.mozilla.org/seamonkey/source/netwerk/base/public/nsIRequestObserver.idl
<bz> hmm
<bz> yeah
<bz> We should document this more clearly.
> yeah this is wrong
> onStopRequest clearly says you have to call onStartRequest

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/netwerk/base/public/nsIRequestObserver.idl&rev=1.5&mark=63-64,72#48

> oDA doesn't say anything like that
> i actually do read interfaces
<bz> heh

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

You can't put that onto nsIStreamListener.
Consider
http://lxr.mozilla.org/seamonkey/source/netwerk/base/public/nsISimpleStreamListener.idl
Now consider that init will fail.

What now? Surely you can't call OStartR/OStopR on an uninitialized object.

Or what if an asyncOpen on a channel fails. In most cases that will also mean
that OStartR is never called.

What should be documented (I'm not sure whether it is) is that
nsIChannel::asyncOpen will always call OStartR and OStopR, if it succeeds.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Biesi, I think the point is that you have to call onStartRequest before calling
onDataAvailable.  But that's not clear from the interface.  No one is saying
that onStartRequest has to be called in general if you're not doing anything
with the listener...

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #2)
> Biesi, I think the point is that you have to call onStartRequest before calling
> onDataAvailable.

Ah, that makes sense... I missed that part of comment 0, it seems.

Comment 4

[George Veneel Dogga [::TheLayman]]

Hey Patrick, I'm new to open source. 
I've just setup the dev environment for Firefox. 

Can I work on this bug?

Comment 5

[Dragana Damjanovic [:dragana]]

Just go a head. If you need help just ask.

Comment 6

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

This good-first-bug hasn't had any activity for 6 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.

Comment 8

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

The comment in https://searchfox.org/mozilla-central/source/netwerk/base/nsIStreamListener.idl should say something like:

After successfully calling nsIRequest.asyncOpen the nsIRequestObserver.onStartRequest must be called exactly once.
That can be followed by 0 or more calls to onDataAvailable.
After all onDataAvailable calls have been made, nsIRequestObserver.onStopRequest must be called exactly once to signal the request is complete.

Comment 9

[Ruhan]

Hi, can I take this bug? from my understanding we just need to add some comments to nsIStreamListener's documentation?

Comment 10

[edgul]

Yes, go ahead

Comment 11

[Ruhan]

(In reply to Valentin Gosu [:valentin] (he/him) from comment #8)

The comment in https://searchfox.org/mozilla-central/source/netwerk/base/nsIStreamListener.idl should say something like:

After successfully calling nsIRequest.asyncOpen the nsIRequestObserver.onStartRequest must be called exactly once.
That can be followed by 0 or more calls to onDataAvailable.
After all onDataAvailable calls have been made, nsIRequestObserver.onStopRequest must be called exactly once to signal the request is complete.

Is this the only comment that needs to be added to the documentation or are additional comments necessary?

Comment 12

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

Yes. There's a similar comment already present on nsIChannel::asyncOpen, but we should also have some info in nsIStreamListener.

Comment 13

[Ruhan]

Attachment: Bug 242313 - docummented that nsiStreamListener calls onsStartRequest and onStopRequest. r=valentin

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cfe8a9eb211b
docummented that nsiStreamListener calls onsStartRequest and onStopRequest. r=valentin,necko-reviewers DONTBUILD

Comment 15

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/cfe8a9eb211b