Remove nsIHttpChannelInternal.responseSynthesized in favor of nsILoadInfo.serviceWorkerTaintingSynthesized

RESOLVED FIXED in Firefox 62

Status

()

enhancement
P2
normal
RESOLVED FIXED
10 months ago
3 months ago

People

(Reporter: bkelly, Assigned: bkelly)

Tracking

(Blocks 1 bug)

unspecified
mozilla62
Points:
---

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

10 months ago
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.
(Assignee)

Comment 3

10 months ago
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.
Attachment #8984843 - Flags: review?(valentin.gosu)
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?
Attachment #8984843 - Flags: review?(valentin.gosu) → review+
(Assignee)

Comment 5

10 months ago
(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 7

10 months ago
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/973e2ce95b63
Remove nsIHttpChannelInternal.responseSynthesized and use nsILoadInfo.serviceWorkerTaintingSynthesized instead. r=valentin

Updated

10 months ago
Priority: -- → P2

Comment 8

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/973e2ce95b63
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.