1468209 - Remove nsIHttpChannelInternal.responseSynthesized in favor of nsILoadInfo.serviceWorkerTaintingSynthesized

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, enhancement, P2)

Description

[Ben Kelly [:bkelly, not reviewing]]

Currently we have two different booleans that are tracked on channels, but are always the same value:

  nsIHttpChannelInternal.responseSynthesized
  nsILoadInfo.serviceWorkerTaintingSynthesized

Lets get rid of the nsIHttpChannelInternal boolean and just use the nsILoadInfo value.

This will help green up the service worker tests with the pref flipped because its one less piece of information that needs to be propagated back to the child from the parent.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Remove nsIHttpChannelInternal.responseSynthesized and use nsILoadInfo.serviceWorkerTaintingSynthesized instead. r=valentin

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f5fef920f20c05fbfa2c25f9d6484d870eed0907

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8984843 [details] [diff] [review]
Remove nsIHttpChannelInternal.responseSynthesized and use nsILoadInfo.serviceWorkerTaintingSynthesized instead. r=valentin

Valentin, this patch removes some duplicate information we are storing between the channel and the LoadInfo.  Way back when we first added nsIHttpChannelInternal.responseSynthesized to indicate if a channel was synthesized by a service worker.  We then later added nsILoadInfo.serviceWorkerTaintingSynthesized to reflect if the tainting value was set when the service worker synthesized a response.  These values should always be the same.

In order to make parent-side intercept work I need to propagate this information back to the child process.  I could propagate both values, but it would be better to remove the duplicate information and just copy one across.

So this patch removes the older nsIHttpChannelInternal.responseSynthesized in favor of the newer nsILoadInfo value.

Comment 4

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

Comment on attachment 8984843 [details] [diff] [review]
Remove nsIHttpChannelInternal.responseSynthesized and use nsILoadInfo.serviceWorkerTaintingSynthesized instead. r=valentin

Review of attachment 8984843 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +472,5 @@
>  NS_IMETHODIMP
>  HttpBaseChannel::SetLoadFlags(nsLoadFlags aLoadFlags)
>  {
> +  bool synthesized = mLoadInfo &&
> +                     mLoadInfo->GetServiceWorkerTaintingSynthesized();

Do we need to ensure that SetLoadInfo isn't called after SetLoadFlags?

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Valentin Gosu [:valentin] from comment #4)
> ::: netwerk/protocol/http/HttpBaseChannel.cpp
> @@ +472,5 @@
> >  NS_IMETHODIMP
> >  HttpBaseChannel::SetLoadFlags(nsLoadFlags aLoadFlags)
> >  {
> > +  bool synthesized = mLoadInfo &&
> > +                     mLoadInfo->GetServiceWorkerTaintingSynthesized();
> 
> Do we need to ensure that SetLoadInfo isn't called after SetLoadFlags?

Actually, reading the comment it seems we may not need this at all any more.  Let me try removing this code.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Remove nsIHttpChannelInternal.responseSynthesized and use nsILoadInfo.serviceWorkerTaintingSynthesized instead. r=valentin

Local testing looks good with that SetLoadFlags() code removed.  Lets verify with a try build:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f2c58c6f4665d6a2ec5db95368f21729c4082a17

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/973e2ce95b63
Remove nsIHttpChannelInternal.responseSynthesized and use nsILoadInfo.serviceWorkerTaintingSynthesized instead. r=valentin

Comment 8

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/973e2ce95b63