1170097 - Add originAttributesToCookieJar to ChromeUtils.webidl

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

See Bobby's comment from https://bugzilla.mozilla.org/show_bug.cgi?id=1168300#c14

Comment 1

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Patch.

Comment 2

[Bobby Holley (:bholley)]

Comment on attachment 8613426 [details] [diff] [review]
Patch.

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

::: dom/bindings/Bindings.conf
@@ +271,5 @@
>      # collection of static methods, so we have this `concrete: False` hack.
>      'concrete': False,
>      'nativeType': 'mozilla::devtools::ChromeUtils',
> +    'implicitJSContext': ['readHeapSnapshot', 'saveHeapSnapshot',
> +                          'originAttributesToCookieJar']

I don't think you'll need implicitJSContext for this, see further comments.

::: dom/webidl/ChromeUtils.webidl
@@ +34,5 @@
> +   * opaque identfier.
> +   *
> +   * @param originAttrs       The originAttributes for the caller.
> +   */
> +  [ChromeOnly]

This entire interface is ChromeOnly, so this annotation (and the beginning of the comment above) are redundant.

@@ +35,5 @@
> +   *
> +   * @param originAttrs       The originAttributes for the caller.
> +   */
> +  [ChromeOnly]
> +  static DOMString originAttributesToCookieJar(optional OriginAttributesDictionary originAttrs);

Make this return a ByteString so that we don't have to bother with the UTF16 conversion.

::: toolkit/devtools/server/ChromeUtils.cpp
@@ +433,5 @@
> +/* static */ void
> +ChromeUtils::OriginAttributesToCookieJar(GlobalObject& global,
> +                                         JSContext* cx,
> +                                         const OriginAttributesDictionary& originAttrs,
> +                                         nsAString& cookieJar)

These should be called |aGlobal|, |aAttrs|, |aCookieJar|.

@@ +439,5 @@
> +  nsCOMPtr<nsIURI> dummyURI;
> +  nsresult rv = NS_NewURI(getter_AddRefs(dummyURI), "http://example.com");
> +  if (NS_FAILED(rv)) {
> +    return;
> +  }

This is too much of a hack - we're in C++, and we can make things work exactly how we like. Here's what I propose:

(1) Add a copy constructor from OriginAttributesDictionary to OriginAttributes like this:

OriginAttributes(const OriginAttributesDictionary& aOther)
  : OriginAttributesDictionary(aOther) {}

(2) Create a method called OriginAttributes::CookieJar(nsACString&). Have BasePrincipal::GetJarPrefix and BasePrincipal::CookieJar forward to that method.

(3) Reimplement this method as two lines:

OriginAttributes attrs(aAttrs);
aAttrs.CookieJar(aCookieJar);

::: toolkit/devtools/server/ChromeUtils.h
@@ +15,5 @@
>  #include "mozilla/AlreadyAddRefed.h"
>  #include "mozilla/ErrorResult.h"
>  #include "mozilla/dom/BindingDeclarations.h"
>  #include "mozilla/dom/ChromeUtilsBinding.h"
> +#include "mozilla/dom/SystemDictionariesBinding.h"

In this patch, can you also move OriginAttributes from SystemDictionaries to ChromeUtils, and get rid of SystemDictionaries.webidl?

::: toolkit/devtools/server/tests/unit/test_originAttributesToCookieJar.js
@@ +5,5 @@
> +  const appId = 12;
> +
> +  var cookieJar_1 = ChromeUtils.originAttributesToCookieJar({appId: appId,
> +                                                             inBrowser: true});
> +  do_check_eq(cookieJar_1, appId + "+t+");

Because the cookieJar representation is going to change, we shouldn't hard-code it here. Instead, we should just make sure that we get the same result with this helper as we do when creating a principal with the same OriginAttributes and accessing .cookieJar.

Comment 3

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 2: Move OriginAttributeDictionary.

Comment 4

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 2: Add originAttributesToCookieJar

Comment 5

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8614613 [details] [diff] [review]
Part 2: Add originAttributesToCookieJar

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

::: toolkit/devtools/server/ChromeUtils.cpp
@@ +10,5 @@
>  
>  #include "mozilla/devtools/HeapSnapshot.h"
>  #include "mozilla/devtools/ZeroCopyNSIOutputStream.h"
>  #include "mozilla/Attributes.h"
> +#include "mozilla/BasePrincipal.h"

Right now this line will cause some compilation error
will looking into this and attach another patch for fixing this.


 0:08.61 In file included from ../../../dist/include/nsISupportsUtils.h:14:0,
 0:08.61                  from ../../../dist/include/nsCOMPtr.h:30,
 0:08.61                  from ../../../dist/include/mozilla/dom/BindingDeclarations.h:20,
 0:08.61                  from /home/allstars/src/mozilla/mozilla-central/toolkit/devtools/server/ChromeUtils.h:17,
 0:08.61                  from /home/allstars/src/mozilla/mozilla-central/toolkit/devtools/server/ChromeUtils.cpp:6:
 0:08.61 ../../../dist/include/nsCOMPtr.h: In instantiation of ‘nsCOMPtr<T>::~nsCOMPtr() [with T = nsIContentSecurityPolicy]’:
 0:08.61 ../../../dist/include/mozilla/BasePrincipal.h:64:19:   required from here
 0:08.62 ../../../dist/include/nsISupportsImpl.h:215:60: error: invalid static_cast from type ‘nsIContentSecurityPolicy*’ to type ‘nsISupports*’
 0:08.62      NS_LogCOMPtrRelease((_c), static_cast<nsISupports*>(_p))
 0:08.62                                                             ^
 0:08.62 ../../../dist/include/nsCOMPtr.h:389:5: note: in expansion of macro ‘NSCAP_LOG_RELEASE’
 0:08.62      NSCAP_LOG_RELEASE(this, mRawPtr);
 0:08.62      ^
 0:08.62 In file included from ../../../dist/include/mozilla/dom/BindingDeclarations.h:20:0,
 0:08.62                  from /home/allstars/src/mozilla/mozilla-central/toolkit/devtools/server/ChromeUtils.h:17,
 0:08.62                  from /home/allstars/src/mozilla/mozilla-central/toolkit/devtools/server/ChromeUtils.cpp:6:
 0:08.62 ../../../dist/include/nsCOMPtr.h:93:39: error: invalid use of incomplete type ‘class nsIContentSecurityPolicy’
 0:08.62    #define NSCAP_RELEASE(this, ptr)    (ptr)->Release()
 0:08.62                                        ^
 0:08.62 ../../../dist/include/nsCOMPtr.h:391:7: note: in expansion of macro ‘NSCAP_RELEASE’
 0:08.62        NSCAP_RELEASE(this, mRawPtr);
 0:08.62        ^
 0:08.62 In file included from ../../../dist/include/mozilla/BasePrincipal.h:10:0,
 0:08.62                  from /home/allstars/src/mozilla/mozilla-central/toolkit/devtools/server/ChromeUtils.cpp:14:
 0:08.62 ../../../dist/include/nsIPrincipal.h:27:7: error: forward declaration of ‘class nsIContentSecurityPolicy’
 0:08.62  class nsIContentSecurityPolicy; /* forward declaration */
 0:08.62        ^
 0:08.92 
 0:08.92 In the directory  /home/allstars/src/mozilla/mozilla-central/obj-x86_64-unknown-linux-gnu/toolkit/devtools/server
 0:08.92 The following command failed to execute properly:

Comment 6

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Bobby Holley (:bholley) from comment #2)
> Comment on attachment 8613426 [details] [diff] [review]
> Patch.
>
Thanks for the great comment, Bobby.
I've addressed your comments but still need some time to look into comment 5
once it's fixed I'll send another r?.

Thanks

Comment 7

[Bobby Holley (:bholley)]

The BasePrincipal destructor needs to move to BasePrincipal.cpp. Baku also does this in his patch in bug 1155153, but you can add it here too and whoever lands second can resolve the conflicts. :-)

Comment 8

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 3: Add originAttributesToCookieJar

fix some typo.

Comment 9

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 1: move cstor/dstor to BasePrincipal.cpp

Comment 10

[Bobby Holley (:bholley)]

Comment on attachment 8615067 [details] [diff] [review]
Part 3: Add originAttributesToCookieJar

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

perfect.

Comment 11

[Bobby Holley (:bholley)]

Comment on attachment 8615068 [details] [diff] [review]
Part 1: move cstor/dstor to BasePrincipal.cpp

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

Nit - this should probably be re-ordered as part 1 so that each commit compiles during bisection.

Comment 12

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 1: move cstor/dstor to BasePrincipal.cpp

Comment 13

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 1: Move OriginAttributeDictionary.

Comment 14

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 3: Add originAttributesToCookieJar

Comment 15

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 3: Add originAttributesToCookieJar

rebase to fix conflict

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/c9c602344f56
https://hg.mozilla.org/integration/b2g-inbound/rev/78ef803f4f11

Comment 17

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Pulsebot from comment #16)
https://hg.mozilla.org/integration/b2g-inbound/rev/8aaa0a247a6f
> https://hg.mozilla.org/integration/b2g-inbound/rev/c9c602344f56
> https://hg.mozilla.org/integration/b2g-inbound/rev/78ef803f4f11

Comment 18

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/b2g-inbound/rev/e9285dba7ba6
https://hg.mozilla.org/integration/b2g-inbound/rev/89af4c72fce5

Comment 19

[Carsten Book [:Tomcat]]

Hi,

part 1 had a wrong bug number in it :)

also had to back this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=2107407&repo=b2g-inbound

Comment 20

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8615851 [details] [diff] [review]
Part 3: Add originAttributesToCookieJar

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

::: caps/BasePrincipal.h
@@ +26,5 @@
>    {
>      mAppId = aAppId;
>      mInBrowser = aInBrowser;
>    }
> +  OriginAttributes(const OriginAttributesDictionary& aOther)

need to add 'explicit' to prevent clang-static-analysis error 
see https://treeherder.mozilla.org/logviewer.html#?job_id=8308189&repo=try

Comment 21

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 1: move cstor/dstor to BasePrincipal.cpp

fixed wrong bug number. :P

Comment 22

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 2: Add originAttributesToCookieJar v2

add 'explicit' to fix clang-static-anylysis error per Comment 36.
Bobby, sorry for bothering you again
can you help to review this patch again?

Thanks

Comment 23

[Bobby Holley (:bholley)]

Comment on attachment 8617073 [details] [diff] [review]
Part 2: Add originAttributesToCookieJar v2

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

explicit was the only thing added right? No need to re-request review for trivial changes. :-)

Comment 24

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Bobby Holley (:bholley) from comment #23)
> explicit was the only thing added right? 
Yes
> No need to re-request review for
> trivial changes. :-)
okay, thanks for the quick review.

Comment 25

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Hi, in bug 1149294 I am moving the ChromeUtils implementation into dom/base as well as splitting out the thread-safe methods to ThreadSafeChromeUtils, and I think you will probably need to rebase on top of my patches. Sorry! Hope it isn't too inconvenient...

Comment 26

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/e0c3fc205ceb
https://hg.mozilla.org/integration/b2g-inbound/rev/3119d57ebf19

Comment 27

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #25)
> Hi, in bug 1149294 I am moving the ChromeUtils implementation into dom/base
> as well as splitting out the thread-safe methods to ThreadSafeChromeUtils,
> and I think you will probably need to rebase on top of my patches. Sorry!
> Hope it isn't too inconvenient...

Hi Nick
Sorry I plan to push it already but the tree is close recently. In your bug there are still dicussions(ni?) going on, I thought it may take a while so I just pushed my commits.

Sorry for the inconvenience.

Comment 28

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Yoshi Huang[:allstars.chh] from comment #27)
> (In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #25)
> > Hi, in bug 1149294 I am moving the ChromeUtils implementation into dom/base
> > as well as splitting out the thread-safe methods to ThreadSafeChromeUtils,
> > and I think you will probably need to rebase on top of my patches. Sorry!
> > Hope it isn't too inconvenient...
> 
> Hi Nick
> Sorry I plan to push it already but the tree is close recently. In your bug
> there are still dicussions(ni?) going on, I thought it may take a while so I
> just pushed my commits.
> 
> Sorry for the inconvenience.

That's fine, I thought I was closer to landing than I turned out to be :)

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e0c3fc205ceb
https://hg.mozilla.org/mozilla-central/rev/3119d57ebf19