1168208 - Refactor the existing logic for syncing the security info between Response and channel objects into a new helper class

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[(no longer active)]

I will introduce a new ChannelInfo object in this bug that will have only one member for the security info for now.  We may want to store other things in it later.

Comment 2

[(no longer active)]

Attachment: Refactor the existing logic for syncing the security info between Response and channel objects into a new helper class

Asking review from Ben on the Cache parts, Josh on the intercepted channel parts, and Nikhil on everything else.

Note that this patch should not change the existing behavior of anything, it is merely a refactoring of the security info nsCString members stored all over the place into ChannelInfo.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8610243 [details] [diff] [review]
Refactor the existing logic for syncing the security info between Response and channel objects into a new helper class

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

Would it be worth sticking something like this in DBSchema to make sure its updated when ChannelInfo changes?

  static_assert(sizeof(IPCChannelInfo) == sizeof(nsCString), "ChannelInfo should have one field");

::: dom/fetch/ChannelInfo.h
@@ +69,5 @@
> +private:
> +  void SetSecurityInfo(nsISupports* aSecurityInfo);
> +
> +private:
> +  nsCString mSecurityInfo;

You could avoid duplicating the definition of the underlying data type by putting an IPCChannelInfo here instead of inlining fields like mSecurityInfo again.

Writing a serializer for ChannelInfo is the other option to avoid code duplication.

Comment 4

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8610243 [details] [diff] [review]
Refactor the existing logic for syncing the security info between Response and channel objects into a new helper class

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

::: dom/fetch/ChannelInfo.h
@@ +19,5 @@
> +namespace dom {
> +
> +// This class represents the information related to a Response that we
> +// retrieve from the corresponding channel that is used to perform the fetch.
> +// Note: when adding new members to this object, the following code needs to

Nit: Get rid of 'Note:' and move this to a separate paragraph instead.

@@ +27,5 @@
> +// * ResurrectInfoOnChannel member
> +// * AsIPCChannelInfo member
> +// * constructors and assignment operators for this class.
> +// * DOM Cache schema code (in dom/cache/DBSchema.cpp) to ensure that the newly
> +//   added member is saved into the DB and loaded from it properly.

Please add a comment about how this should be used by both setters and getters considering we fail hard if !mInited.

@@ +36,5 @@
> +
> +  ChannelInfo()
> +    : mInited(false)
> +  {
> +  }

Nit: newline.

::: netwerk/protocol/http/nsHttpConnectionInfo.h
@@ +10,5 @@
>  #include "nsHttp.h"
>  #include "nsProxyInfo.h"
>  #include "nsCOMPtr.h"
>  #include "nsStringFwd.h"
> +#include "mozilla/Logging.h"

Different patch?

Comment 5

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

(In reply to Ben Kelly [:bkelly] from comment #3)
> Comment on attachment 8610243 [details] [diff] [review]
> Refactor the existing logic for syncing the security info between Response
> and channel objects into a new helper class
> 
> Review of attachment 8610243 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Would it be worth sticking something like this in DBSchema to make sure its
> updated when ChannelInfo changes?
> 
>   static_assert(sizeof(IPCChannelInfo) == sizeof(nsCString), "ChannelInfo
> should have one field");

Unfortunately mInited will make this fail.

Comment 6

[(no longer active)]

(In reply to Ben Kelly [:bkelly] from comment #3)
> Would it be worth sticking something like this in DBSchema to make sure its
> updated when ChannelInfo changes?
> 
>   static_assert(sizeof(IPCChannelInfo) == sizeof(nsCString), "ChannelInfo
> should have one field");

What Nikhil said.  :/

> ::: dom/fetch/ChannelInfo.h
> @@ +69,5 @@
> > +private:
> > +  void SetSecurityInfo(nsISupports* aSecurityInfo);
> > +
> > +private:
> > +  nsCString mSecurityInfo;
> 
> You could avoid duplicating the definition of the underlying data type by
> putting an IPCChannelInfo here instead of inlining fields like mSecurityInfo
> again.
> 
> Writing a serializer for ChannelInfo is the other option to avoid code
> duplication.

I would like to delay doing anything here until we know what else we would like to store here, and will then consider merging these types...

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #4)
> ::: netwerk/protocol/http/nsHttpConnectionInfo.h
> @@ +10,5 @@
> >  #include "nsHttp.h"
> >  #include "nsProxyInfo.h"
> >  #include "nsCOMPtr.h"
> >  #include "nsStringFwd.h"
> > +#include "mozilla/Logging.h"
> 
> Different patch?

This is needed in order to be able to #include "HttpBaseChannel.h" standalone, which is needed in ChannelInfo.cpp.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ae9c77fa58d1

Comment 8

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=10190761&repo=mozilla-inbound

Comment 9

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7431168bfa3b

Comment 10

[(no longer active)]

Bug 1161684 broke this... Trying to fix right now.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/37688076473b

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/37688076473b