1325915 - remove invocation order dependency on OnProgress and OnDataAvailable in HttpChannelParent

Status: RESOLVED FIXED

(Core :: Networking, defect)

Comment 1

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Bug 1325915 - Part 1, derive transportStatus in HttpChannelParent ODA and derive progress/progressMax in child side.

Review commit: https://reviewboard.mozilla.org/r/101054/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/101054/

Comment 2

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Currently, HttpChannelParent depends on OnStatus/OnProgress to be called before OnDataAvailable, in order to aggregate OnStatus/OnProgress/OnDataAvailable in one IPC message. We can remove this dependency by deriving necessary information in ODA directly.

[For OnProgress]
https://searchfox.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#6953

progressMax: can get from response header in child process
progress: can be derived from `offset` and `count` from ODA

[For OnStatus]
https://searchfox.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#6942

transportStatus: can be derived from `nsHttpChannel::IsFromCache`

Comment 3

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8821986 [details]
Bug 1325915 - Part 1, derive transportStatus in HttpChannelParent ODA and derive progress/progressMax in child side.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/101054/diff/1-2/

Comment 4

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8821986 [details]
Bug 1325915 - Part 1, derive transportStatus in HttpChannelParent ODA and derive progress/progressMax in child side.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/101054/diff/2-3/

Comment 5

[Honza Bambas (:mayhemer)]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #2)
> Currently, HttpChannelParent depends on OnStatus/OnProgress to be called
> before OnDataAvailable, in order to aggregate
> OnStatus/OnProgress/OnDataAvailable in one IPC message. We can remove this
> dependency by deriving necessary information in ODA directly.
> 
> [For OnProgress]
> https://searchfox.org/mozilla-central/source/netwerk/protocol/http/
> nsHttpChannel.cpp#6953

typical problem when you don't refer a permalink (with revision).  this link may no longer point where you want.

> 
> progressMax: can get from response header in child process
> progress: can be derived from `offset` and `count` from ODA
> 
> [For OnStatus]
> https://searchfox.org/mozilla-central/source/netwerk/protocol/http/
> nsHttpChannel.cpp#6942
> 
> transportStatus: can be derived from `nsHttpChannel::IsFromCache`

This needs double checking for partial content: NS_NET_STATUS_READING is reported for the part we read from the cache, NS_NET_STATUS_RECEIVING_FROM for the rest coming from the network.  I'm not sure nsHttpChannel::IsFromCache returns exactly what you want.

Comment 6

[Honza Bambas (:mayhemer)]

Comment on attachment 8821986 [details]
Bug 1325915 - Part 1, derive transportStatus in HttpChannelParent ODA and derive progress/progressMax in child side.

https://reviewboard.mozilla.org/r/101054/#review105736

I miss good comments, r+ with that addressed

::: netwerk/protocol/http/HttpChannelParent.h:222
(Diff revision 3)
>    nsCOMPtr<nsIAsyncVerifyRedirectCallback> mRedirectCallback;
>  
>    nsAutoPtr<class nsHttpChannel::OfflineCacheEntryAsForeignMarker> mOfflineForeignMarker;
>  
> -  // state for combining OnStatus/OnProgress with OnDataAvailable
> -  // into one IPDL call to child.
> +  // Ignore OnProgress if precedes ODA
> +  bool mIgnoreProgress = false;

say when and why it's set

::: netwerk/protocol/http/HttpChannelParent.cpp:1321
(Diff revision 3)
>  {
> -  // If this precedes OnDataAvailable, store and ODA will send to child.
> +  // If this precedes OnDataAvailable, transportStatus will be derived in ODA.
>    if (aStatus == NS_NET_STATUS_RECEIVING_FROM ||
>        aStatus == NS_NET_STATUS_READING)
>    {
> -    mStoredStatus = aStatus;
> +    mIgnoreProgress = true;

why?

Comment 7

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Honza Bambas (:mayhemer) from comment #5)
> (In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment
> #2)
> > Currently, HttpChannelParent depends on OnStatus/OnProgress to be called
> > before OnDataAvailable, in order to aggregate
> > OnStatus/OnProgress/OnDataAvailable in one IPC message. We can remove this
> > dependency by deriving necessary information in ODA directly.
> > 
> > [For OnProgress]
> > https://searchfox.org/mozilla-central/source/netwerk/protocol/http/
> > nsHttpChannel.cpp#6953
> 
> typical problem when you don't refer a permalink (with revision).  this link
> may no longer point where you want.
permalink: https://searchfox.org/mozilla-central/rev/0aed9484bd3e97206fd1949ee4a4992ef300a81f/netwerk/protocol/http/nsHttpChannel.cpp#6964
> 
> > 
> > progressMax: can get from response header in child process
> > progress: can be derived from `offset` and `count` from ODA
> > 
> > [For OnStatus]
> > https://searchfox.org/mozilla-central/source/netwerk/protocol/http/
> > nsHttpChannel.cpp#6942
> > 
> > transportStatus: can be derived from `nsHttpChannel::IsFromCache`
> 
> This needs double checking for partial content: NS_NET_STATUS_READING is
> reported for the part we read from the cache, NS_NET_STATUS_RECEIVING_FROM
> for the rest coming from the network.  I'm not sure
> nsHttpChannel::IsFromCache returns exactly what you want.
permalink: https://searchfox.org/mozilla-central/rev/0aed9484bd3e97206fd1949ee4a4992ef300a81f/netwerk/protocol/http/nsHttpChannel.cpp#6954

I didn't aware that OnDataAvailable in one nsHttpChannel object can be called partially by cache. In this case, nsHttpChannel::IsFromCache will not reflect the same logic. The only solution I can think of is storing another state in nsHttpChannel.

Comment 8

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8821986 [details]
Bug 1325915 - Part 1, derive transportStatus in HttpChannelParent ODA and derive progress/progressMax in child side.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/101054/diff/3-4/

Comment 9

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8821986 [details]
Bug 1325915 - Part 1, derive transportStatus in HttpChannelParent ODA and derive progress/progressMax in child side.

https://reviewboard.mozilla.org/r/101054/#review105736

> say when and why it's set

comment added in next revision.

> why?

comment added in next revision.

Comment 10

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8821986 [details]
Bug 1325915 - Part 1, derive transportStatus in HttpChannelParent ODA and derive progress/progressMax in child side.

patch updated according to review comment.

Comment 11

[Honza Bambas (:mayhemer)]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #7)
> I didn't aware that OnDataAvailable in one nsHttpChannel object can be
> called partially by cache. In this case, nsHttpChannel::IsFromCache will not
> reflect the same logic. The only solution I can think of is storing another
> state in nsHttpChannel.

And can't you find our from the aRequest == mCachePump or aRequest == mTransactionPump in ODA?

Comment 12

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Honza Bambas (:mayhemer) from comment #11)
> (In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment
> #7)
> > I didn't aware that OnDataAvailable in one nsHttpChannel object can be
> > called partially by cache. In this case, nsHttpChannel::IsFromCache will not
> > reflect the same logic. The only solution I can think of is storing another
> > state in nsHttpChannel.
> 
> And can't you find our from the aRequest == mCachePump or aRequest ==
> mTransactionPump in ODA?

I cannot do that with only the parameter passing to HttpChannelParent::OnDataAvailable. Need to store extra information in nsHttpChannel. See my latest revision for the actual code could look like.

Comment 13

[Honza Bambas (:mayhemer)]

Comment on attachment 8821986 [details]
Bug 1325915 - Part 1, derive transportStatus in HttpChannelParent ODA and derive progress/progressMax in child side.

https://reviewboard.mozilla.org/r/101054/#review105904

::: netwerk/protocol/http/HttpChannelChild.cpp:1204
(Diff revisions 3 - 4)
>    // Hold a ref to this to keep it from being deleted by Send__delete__()
>    RefPtr<HttpChannelChild> self(this);
>    Send__delete__(this);
>  
> +  // Reset the event target to which queued messages are delivered. Otherwise
> +  // we'll get an assertion when we re-use the channel later on.

this is not really a sufficient explanation

::: netwerk/protocol/http/HttpChannelParent.h:225
(Diff revision 4)
>    nsCOMPtr<nsIAsyncVerifyRedirectCallback> mRedirectCallback;
>  
>    nsAutoPtr<class nsHttpChannel::OfflineCacheEntryAsForeignMarker> mOfflineForeignMarker;
>  
> -  // state for combining OnStatus/OnProgress with OnDataAvailable
> -  // into one IPDL call to child.
> +  // OnStatus is always called before OnProgress.
> +  // Set true in OnStatus if next OnProgress can be ignored.

and why it can be ignored?

::: netwerk/protocol/http/HttpChannelParent.h:226
(Diff revision 4)
>  
>    nsAutoPtr<class nsHttpChannel::OfflineCacheEntryAsForeignMarker> mOfflineForeignMarker;
>  
> -  // state for combining OnStatus/OnProgress with OnDataAvailable
> -  // into one IPDL call to child.
> -  nsresult mStoredStatus;
> +  // OnStatus is always called before OnProgress.
> +  // Set true in OnStatus if next OnProgress can be ignored.
> +  bool mIgnoreProgress = false;

make this a bit field if it's single thread used.  if not, it should be atomic.  otherwise, assert thread when accessed

::: netwerk/protocol/http/HttpChannelParent.cpp:1303
(Diff revision 4)
> -    mStoredProgressMax = aProgressMax;
> -  } else {
> -    // Send OnProgress events to the child for data upload progress notifications
> +  // Send OnProgress events to the child for data upload progress notifications
> -    // (i.e. status == NS_NET_STATUS_SENDING_TO) or if the channel has
> +  // (i.e. status == NS_NET_STATUS_SENDING_TO) or if the channel has
> -    // LOAD_BACKGROUND set.
> +  // LOAD_BACKGROUND set.
> -    if (mIPCClosed || !SendOnProgress(aProgress, aProgressMax))
> +  if (mIPCClosed || !SendOnProgress(aProgress, aProgressMax)) {

can this ever be called with the mIgnoreProgress logic?  if yes, when?  if not, why not to ignore this completely?

::: netwerk/protocol/http/nsHttpChannel.h:272
(Diff revision 4)
>      bool AwaitingCacheCallbacks();
>      void SetCouldBeSynthesized();
>  
> +    // Return true if the latest ODA is invoked by mCachePump.
> +    // Should only be called on the same thread as ODA.
> +    bool IsReadingFromCache() const { return mIsReadingFromCache; }

assert main thread

Comment 14

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8821986 [details]
Bug 1325915 - Part 1, derive transportStatus in HttpChannelParent ODA and derive progress/progressMax in child side.

https://reviewboard.mozilla.org/r/101054/#review105904

> this is not really a sufficient explanation

This is done by @billm in Bug 1318506, do you want me to fix the comment as well?

> and why it can be ignored?

I plan to change the comment into:
```
// OnStatus is always called before OnProgress.
// Set true in OnStatus if next OnProgress can be ignored
// since the information can be recontructed from ODA.
```

> make this a bit field if it's single thread used.  if not, it should be atomic.  otherwise, assert thread when accessed

This is main thread only, I'll make it a bit field.

> can this ever be called with the mIgnoreProgress logic?  if yes, when?  if not, why not to ignore this completely?

upload will also produce onprogress/onstatus event so we cannot ignore this completely.

> assert main thread

`IsReadingFromCache` will be called on the retargeted thread, I'll change mIsReadingFromCache to Atomic<bool>.

Comment 15

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8821986 [details]
Bug 1325915 - Part 1, derive transportStatus in HttpChannelParent ODA and derive progress/progressMax in child side.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/101054/diff/4-5/

Comment 16

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8821986 [details]
Bug 1325915 - Part 1, derive transportStatus in HttpChannelParent ODA and derive progress/progressMax in child side.

@mayhemer you may double check my latest revision along with comment #14.

Comment 17

[Honza Bambas (:mayhemer)]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #14)
> Comment on attachment 8821986 [details]
> Bug 1325915 - derive transportStatus in HttpChannelParent ODA and derive
> progress/progressMax in child side.
> 
> https://reviewboard.mozilla.org/r/101054/#review105904
> 
> > this is not really a sufficient explanation
> 
> This is done by @billm in Bug 1318506, do you want me to fix the comment as
> well?

If you can come up with something better, yes.  But don't get blocked by it.  


> 
> > and why it can be ignored?
> 
> I plan to change the comment into:
> ```
> // OnStatus is always called before OnProgress.
> // Set true in OnStatus if next OnProgress can be ignored
> // since the information can be recontructed from ODA.
> ```
> 
> > make this a bit field if it's single thread used.  if not, it should be atomic.  otherwise, assert thread when accessed
> 
> This is main thread only, I'll make it a bit field.
> 
> > can this ever be called with the mIgnoreProgress logic?  if yes, when?  if not, why not to ignore this completely?
> 
> upload will also produce onprogress/onstatus event so we cannot ignore this
> completely.
> 
> > assert main thread
> 
> `IsReadingFromCache` will be called on the retargeted thread, I'll change
> mIsReadingFromCache to Atomic<bool>.


Thanks.

Comment 18

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Bug 1325915 - Part 2, update comment for reseting event target of event queue after the completion of ServiceWorker interception.

Review commit: https://reviewboard.mozilla.org/r/110288/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/110288/

Comment 19

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8821986 [details]
Bug 1325915 - Part 1, derive transportStatus in HttpChannelParent ODA and derive progress/progressMax in child side.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/101054/diff/5-6/

Comment 20

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Honza Bambas (:mayhemer) from comment #17)
> (In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment
> #14)
> > Comment on attachment 8821986 [details]
> > Bug 1325915 - derive transportStatus in HttpChannelParent ODA and derive
> > progress/progressMax in child side.
> > 
> > https://reviewboard.mozilla.org/r/101054/#review105904
> > 
> > > this is not really a sufficient explanation
> > 
> > This is done by @billm in Bug 1318506, do you want me to fix the comment as
> > well?
> 
> If you can come up with something better, yes.  But don't get blocked by it.
Additional patch to update the comment based on my understanding.

Comment 21

[Honza Bambas (:mayhemer)]

Comment on attachment 8834307 [details]
Bug 1325915 - Part 2, update comment for reseting event target of event queue after the completion of ServiceWorker interception.

https://reviewboard.mozilla.org/r/110288/#review112474

::: netwerk/protocol/http/HttpChannelChild.cpp:1209
(Diff revision 1)
>    // Hold a ref to this to keep it from being deleted by Send__delete__()
>    RefPtr<HttpChannelChild> self(this);
>    Send__delete__(this);
>  
> -  // Reset the event target to which queued messages are delivered. Otherwise
> -  // we'll get an assertion when we re-use the channel later on.
> +  // Reset the event target since the IPC actor is about to be destroyed.
> +  // Following channel event should be handled on main thread.

I honestly still don't understand.  Maybe there is a need to study the code more in detail to follow what's going on here?

Never mind, if you believe this is enough, let's land and not block the bug.

Comment 22

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8834307 [details]
Bug 1325915 - Part 2, update comment for reseting event target of event queue after the completion of ServiceWorker interception.

https://reviewboard.mozilla.org/r/110288/#review112474

> I honestly still don't understand.  Maybe there is a need to study the code more in detail to follow what's going on here?
> 
> Never mind, if you believe this is enough, let's land and not block the bug.

Yeah the combination of Quantum DOM and ServiceWorker makes the code hard to understand. Quantum DOM adding more event targets on main thread and try move Necko IPC to its own event target. However, when ServiceWorker kicks in, it will rewire the channel event generation back to the original event target on main thread.

Leave a reference to Quantum DOM wiki here for further study.
https://wiki.mozilla.org/Quantum/DOM

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3593cac8732b
Part 1, derive transportStatus in HttpChannelParent ODA and derive progress/progressMax in child side. r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/05ea4a3d1f5a
Part 2, update comment for reseting event target of event queue after the completion of ServiceWorker interception. r=mayhemer

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3593cac8732b
https://hg.mozilla.org/mozilla-central/rev/05ea4a3d1f5a