Closed Bug 1724950 Opened 5 years ago Closed 4 years ago

OnStopRequest doesn't seem to be always called after Cancel

Tracking

()

Status:

RESOLVED INVALID

People

(Reporter: julienw, Assigned: kershaw)

References

Details

(Whiteboard: [necko-triaged])

Attachments

(2 files)

log.txt-main.484480.moz_log.gz 5 years ago Julien Wajsberg [:julienw] (PTO -> Aug 24) 4.67 MB, application/gzip		Details
browser_test_marker_network_cancel.js 5 years ago Julien Wajsberg [:julienw] (PTO -> Aug 24) 2.28 KB, application/x-javascript		Details

Julien Wajsberg [:julienw] (PTO -> Aug 24)

Reporter

Description

•

5 years ago

Attached file log.txt-main.484480.moz_log.gz — Details

Here is my STR:

Open cnn.com.
Press "escape" quickly while it's loading.

According to the profiler output and some local tests I made, we enter AsyncOpen with a NS_OK result, but never enter OnStopRequest.

Julien Wajsberg [:julienw] (PTO -> Aug 24)

Reporter

Comment 1

•

5 years ago

Attached file browser_test_marker_network_cancel.js — Details

I also made this pernosco session out of this test file.

Julien Wajsberg [:julienw] (PTO -> Aug 24)

Reporter

Comment 2

•

5 years ago

According to the profiler, here are some URLs that don't seem to go through onStopRequest in the previous log:

I believe some of them were blocked by ublock or Firefox' tracking protection (which, I think, go through CancelInternal as well?), but at least the images at the end were probably canceled by my pressing of escape.

Kershaw Chang [:kershaw]

Assignee

Comment 3

•

5 years ago

I'll investigate this.

Another question to Julien: I assume that OnStopRequest is not called in content process, right? If so, could you also upload the http log from content processes? Thanks.

Assignee: nobody → kershaw

Severity: -- → S3

Flags: needinfo?(felash)

Priority: -- → P2

Whiteboard: [necko-triaged]

Julien Wajsberg [:julienw] (PTO -> Aug 24)

Reporter

Comment 4

•

5 years ago

The content processes look right actually (according to the profiler still) !

I'm not sure why, the logs I got weren't really useful for the child processes, they were very short. I used about:networking#logging to set up the logging, maybe that's why?

Flags: needinfo?(felash)

Julien Wajsberg [:julienw] (PTO -> Aug 24)

Reporter

Comment 5

•

5 years ago

I forgot to mention here: I see the same problems in service workers (so with InterceptedHttpChannel.cpp). This can be tested with profiler.firefox.com itself or lemonde.fr.

Kershaw Chang [:kershaw]

Assignee

Comment 6

•

5 years ago

According to the log, here is what happened when a channel is cancelled:

2021-08-10 14:46:21.164629 UTC - [Parent 484480: Main Thread]: V/nsHttp nsHttpChannel::Cancel [this=7fe85de04000 status=80004004]
2021-08-10 14:46:21.164631 UTC - [Parent 484480: Main Thread]: V/nsHttp 7fe85de04000 called from script: resource://gre/modules/WebRequest.jsm:996:18
2021-08-10 14:46:21.164661 UTC - [Parent 484480: Main Thread]: V/nsHttp nsHttpChannel::ResumeInternal [this=7fe85de04000]
2021-08-10 14:46:21.174773 UTC - [Parent 484480: Main Thread]: D/nsHttp HttpAsyncAborter::AsyncAbort [this=7fe85de04000 status=80004004]
2021-08-10 14:46:21.177871 UTC - [Parent 484480: Main Thread]: V/nsHttp HttpBaseChannel::DoNotifyListener this=7fe85de04000

You can see that HttpBaseChannel::DoNotifyListener is called. This is how we make sure that mListener->OnStopRequest is called. Note that what nsHttpChannel code guarantees is OnStartRequest and OnStopRequest of its mListenr MUST be always called. It's possible that nsHttpChannel::OnStartRequest and nsHttpChannel::OnStopRequest are not called when a channel is cancelled. I think that's the reason that profiler shows that OnStopRequest is not called.

The same logic also applies to InterceptedHttpChannel. It's possible that InterceptedHttpChannel::OnStopRequest won't be called, when AsyncAbort is called here.

So, I think the question is: do we want to also add network markers in HttpBaseChannel::DoNotifyListener?

Flags: needinfo?(felash)

Julien Wajsberg [:julienw] (PTO -> Aug 24)

Reporter

Comment 7

•

5 years ago

No, I don't think we want to do that.

But it's still not clear to me why OnStopRequest isn't called from DoNotifyListener in this case, do you know?

Flags: needinfo?(felash)

Kershaw Chang [:kershaw]

Assignee

Comment 8

•

5 years ago

•

Edited

(In reply to Julien Wajsberg [:julienw] from comment #7)

No, I don't think we want to do that.

But it's still not clear to me why OnStopRequest isn't called from DoNotifyListener in this case, do you know?

Which OnStopRequest? Do you mean nsHttpChannel::OnStopRequest?
Note that DoNotifyListener only calls mListener's OnStartRequest and OnStopRequest.

Julien Wajsberg [:julienw] (PTO -> Aug 24)

Reporter

Comment 9

•

5 years ago

So does that mean that mListener is removed after the cancelation ? Is that normal ?
My understanding was that nsHttpChannel::OnStopRequest should be called in all situations, if AsyncOpen succeeded before that. Is this incorrect?

Kershaw Chang [:kershaw]

Assignee

Comment 10

•

5 years ago

(In reply to Julien Wajsberg [:julienw] from comment #9)

So does that mean that mListener is removed after the cancelation ? Is that normal ?

Not sure if I understand your question. mListener will be released here to make sure that both the channel and mListener can be released.

My understanding was that nsHttpChannel::OnStopRequest should be called in all situations, if AsyncOpen succeeded before that. Is this incorrect?

No, it's not correct. As I said in comment #6, nsHttpChannel::OnStopRequest will not be always called. We only make sure that mListener's OnStartRequest and OnStopRequest will be called in all cases.

Julien Wajsberg [:julienw] (PTO -> Aug 24)

Reporter

Comment 11

•

5 years ago

(In reply to Kershaw Chang [:kershaw] from comment #10)

(In reply to Julien Wajsberg [:julienw] from comment #9)

So does that mean that mListener is removed after the cancelation ? Is that normal ?

Not sure if I understand your question. mListener will be released here to make sure that both the channel and mListener can be released.

My understanding was that nsHttpChannel::OnStopRequest should be called in all situations, if AsyncOpen succeeded before that. Is this incorrect?

No, it's not correct. As I said in comment #6, nsHttpChannel::OnStopRequest will not be always called. We only make sure that mListener's OnStartRequest and OnStopRequest will be called in all cases.

OK, that's what Valentin said in my patch in https://phabricator.services.mozilla.com/D122111, but I guess there's a misunderstanding.

May I ask what object is usually in mListener?

Kershaw Chang [:kershaw]

Assignee

Comment 12

•

5 years ago

OK, that's what Valentin said in my patch in https://phabricator.services.mozilla.com/D122111, but I guess there's a misunderstanding.

I think the OnStopRequest that Valentin meant belongs to mListener, not nsHttpChannel::OnStopRequest.

May I ask what object is usually in mListener?

The mListener here is usually the same as the listener that passes to AsyncOpen function.

Kershaw Chang [:kershaw]

Assignee

Comment 13

•

4 years ago

Close as INVALID, since this is really not an issue.

Status: NEW → RESOLVED

Closed: 4 years ago

Resolution: --- → INVALID

You need to log in before you can comment on or make changes to this bug.

Bugzilla

OnStopRequest doesn't seem to be always called after Cancel

Categories

(Core :: Networking: HTTP, defect, P2)

Tracking

()

People

(Reporter: julienw, Assigned: kershaw)

References

Details

(Whiteboard: [necko-triaged])

Crash Data

Security

(public)

User Story

Attachments

(2 files)

Description

Comment 1

Comment 2

Comment 3

Comment 4

Comment 5

Comment 6

Comment 7

Comment 8

Comment 9

Comment 10

Comment 11

Comment 12

Comment 13

Attachment

General

Description

File Name

Content Type