Add originAttributesToCookieJar to ChromeUtils.webidl

RESOLVED FIXED in Firefox 41

Status

()

Core
DOM
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: allstars, Unassigned)

Tracking

Trunk
mozilla41
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(2 attachments, 9 obsolete attachments)

3.28 KB, patch
allstars
: review+
Details | Diff | Splinter Review
8.61 KB, patch
bholley
: review+
Details | Diff | Splinter Review
(Assignee)

Updated

3 years ago
Attachment #8613426 - Flags: review?(bobbyholley)
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.
Attachment #8613426 - Flags: review?(bobbyholley) → review-
Created attachment 8614612 [details] [diff] [review]
Part 2: Move OriginAttributeDictionary.
Created attachment 8614613 [details] [diff] [review]
Part 2: Add originAttributesToCookieJar
Attachment #8613426 - Attachment is obsolete: true
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:
(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
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. :-)
Created attachment 8615067 [details] [diff] [review]
Part 3: Add originAttributesToCookieJar

fix some typo.
Attachment #8614613 - Attachment is obsolete: true
Created attachment 8615068 [details] [diff] [review]
Part 1: move cstor/dstor to BasePrincipal.cpp
(Assignee)

Updated

3 years ago
Attachment #8614612 - Flags: review?(bobbyholley)
(Assignee)

Updated

3 years ago
Attachment #8615067 - Flags: review?(bobbyholley)
(Assignee)

Updated

3 years ago
Attachment #8615068 - Flags: review?(bobbyholley)
Attachment #8614612 - Flags: review?(bobbyholley) → review+
Comment on attachment 8615067 [details] [diff] [review]
Part 3: Add originAttributesToCookieJar

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

perfect.
Attachment #8615067 - Flags: review?(bobbyholley) → review+
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.
Attachment #8615068 - Flags: review?(bobbyholley) → review+
(Assignee)

Updated

3 years ago
Attachment #8614612 - Attachment description: Part 1: Move OriginAttributeDictionary. → Part 2: Move OriginAttributeDictionary.
(Assignee)

Updated

3 years ago
Attachment #8615067 - Attachment description: Part 2: Add originAttributesToCookieJar → Part 3: Add originAttributesToCookieJar
(Assignee)

Updated

3 years ago
Attachment #8615068 - Attachment description: Part 3: move cstor/dstor to BasePrincipal.cpp → Part 1: move cstor/dstor to BasePrincipal.cpp
Created attachment 8615786 [details] [diff] [review]
Part 1: move cstor/dstor to BasePrincipal.cpp
Attachment #8615068 - Attachment is obsolete: true
Attachment #8615786 - Flags: review+
Created attachment 8615787 [details] [diff] [review]
Part 1: Move OriginAttributeDictionary.
Attachment #8614612 - Attachment is obsolete: true
Attachment #8615787 - Flags: review+
Created attachment 8615788 [details] [diff] [review]
Part 3: Add originAttributesToCookieJar
Attachment #8615067 - Attachment is obsolete: true
Attachment #8615788 - Flags: review+
Created attachment 8615851 [details] [diff] [review]
Part 3: Add originAttributesToCookieJar

rebase to fix conflict
Attachment #8615788 - Attachment is obsolete: true
Attachment #8615851 - Flags: review+
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
Flags: needinfo?(allstars.chh)
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
Created attachment 8617070 [details] [diff] [review]
Part 1: move cstor/dstor to BasePrincipal.cpp

fixed wrong bug number. :P
Attachment #8615786 - Attachment is obsolete: true
Flags: needinfo?(allstars.chh)
Attachment #8617070 - Flags: review+
Created attachment 8617073 [details] [diff] [review]
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
Attachment #8615851 - Attachment is obsolete: true
Attachment #8617073 - Flags: review?(bobbyholley)
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. :-)
Attachment #8617073 - Flags: review?(bobbyholley) → review+
(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.
Summary: Add originAttributesToCookieJar to ChromeUtils.jsm → Add originAttributesToCookieJar to ChromeUtils.webidl
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...
Depends on: 1149294
(Assignee)

Updated

3 years ago
Attachment #8617070 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8615787 - Attachment description: Part 2: Move OriginAttributeDictionary. → Part 1: Move OriginAttributeDictionary.
(Assignee)

Updated

3 years ago
Attachment #8617073 - Attachment description: Part 3: Add originAttributesToCookieJar v2 → Part 2: Add originAttributesToCookieJar v2
(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.
(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 :)
https://hg.mozilla.org/mozilla-central/rev/e0c3fc205ceb
https://hg.mozilla.org/mozilla-central/rev/3119d57ebf19
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox41: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.