make OriginScope use mozilla::Variant

RESOLVED FIXED in Firefox 64

Status

()

P3
normal
RESOLVED FIXED
2 years ago
a month ago

People

(Reporter: froydnj, Assigned: janv, Mentored)

Tracking

Trunk
mozilla64
Points:
---

Firefox Tracking Flags

(firefox53 affected, firefox64 fixed)

Details

(Whiteboard: [lang=c++])

Attachments

(5 attachments, 12 obsolete attachments)

16.85 KB, patch
asuth
: review+
Details | Diff | Splinter Review
14.08 KB, patch
Details | Diff | Splinter Review
8.46 KB, text/plain
Details
18.55 KB, patch
janv
: review+
Details | Diff | Splinter Review
5.37 KB, patch
asuth
: review+
Details | Diff | Splinter Review
(Reporter)

Description

2 years ago
This data structure is the textbook case for when to use Variant.  Perhaps you'd want to use Maybe<Variant<...>> so you can represent the eNull case.
(Assignee)

Comment 1

2 years ago
Yeah, but doesn't nsVariant take much more space in memory ?
(Reporter)

Comment 2

2 years ago
(In reply to Jan Varga [:janv] from comment #1)
> Yeah, but doesn't nsVariant take much more space in memory ?

Did you mean nsVariant or mozilla::Variant?  nsVariant would be unwieldy here, I think.  But mozilla::Variant could handle this with no memory overhead if you wanted to store pointers, or it could just store actual objects, which would make things a little simpler.
Flags: needinfo?(jvarga)
(Assignee)

Comment 3

2 years ago
Oh, I see. mozilla::Variant would be no worse.
Flags: needinfo?(jvarga)

Updated

2 years ago
Priority: -- → P3

Comment 4

2 years ago
I would like to take this bug!
Assignee: nobody → ttung
Status: NEW → ASSIGNED

Comment 5

2 years ago
Created attachment 8823585 [details] [diff] [review]
[WIP] Bug-1324836-v0- Make QriginScope use mozilla::Variant.

Comment 6

2 years ago
Created attachment 8824900 [details] [diff] [review]
Bug-1324836-v1- Make QriginScope use mozilla::Variant.

- Using Maybe<Variant<...>> to replace union{...} for class OriginScope.
- Remove assertions in all the getter functions since these assertions have already been done in Variant::as().
- Remove function Destroy() since the only thing we need to do is call reset().

try : https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ecc331edfb756e428ca92b7a3f4929c394d221c
Attachment #8823585 - Attachment is obsolete: true

Updated

2 years ago
Attachment #8824900 - Flags: feedback?(btseng)

Comment 7

2 years ago
Hi Bevis,

I rewrite the OriginScope as mention in comment #6. Before I ask for the review to :jan, I would like to ask  your feedback about my patch, could you give me some feedback about this? Thanks!
Comment on attachment 8824900 [details] [diff] [review]
Bug-1324836-v1- Make QriginScope use mozilla::Variant.

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

Variant looks quite useful here, but there is still something we could improve. :)
Please see my comment inlined.

Thanks!

::: dom/quota/OriginScope.h
@@ +44,5 @@
>        MOZ_COUNT_DTOR(OriginAndAttributes);
>      }
>    };
>  
> +  using OriginOrPatternOrPrefix =

using (Type|VariantType) shall be good enough.
Or we'll have to keep appending OrXxx if a new type is introduced.

@@ +52,2 @@
>  
> +  mozilla::Maybe<OriginOrPatternOrPrefix> mOriginScope;

ditto

@@ +211,4 @@
>      } else if (IsPrefix()) {
> +      match = StringBeginsWith(
> +                aOther.mOriginScope->as<OriginAndAttributes>().mOrigin,
> +                GetPrefix());

We could probably simply this as:
      match = StringBeginsWith(aOther.GetOrigin(), GetPrefix());

@@ +225,5 @@
>  
>      bool match;
>  
>      if (IsOrigin()) {
> +      match = GetPattern().Matches(GetOriginAttributes());

Boom! We die here because this->IsOrigin() is false. 
We should use aOther.GetPattern() for matching:
      match = aOther.GetPattern().Matches(GetOriginAttributes());

@@ +249,5 @@
>  
>      if (IsOrigin()) {
> +      match = StringBeginsWith(
> +                mOriginScope->as<OriginAndAttributes>().mOrigin,
> +                aOther.GetPrefix());

We could simply this as:
      match = StringBeginsWith(GetOrigin(), aOther.GetPrefix());

@@ +312,5 @@
> +  {
> +    mOriginScope.emplace(mozilla::OriginAttributesPattern());
> +    mozilla::OriginAttributesPattern pattern =
> +      mOriginScope->as<mozilla::OriginAttributesPattern>();
> +    MOZ_ALWAYS_TRUE(pattern.Init(aJSONPattern));

I think we can just call SetFromJSONPattern(aJSONPatten) here.
Attachment #8824900 - Flags: feedback?(btseng)
(Reporter)

Comment 9

2 years ago
Comment on attachment 8824900 [details] [diff] [review]
Bug-1324836-v1- Make QriginScope use mozilla::Variant.

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

This is a great start, thank you.

::: dom/quota/OriginScope.h
@@ +247,4 @@
>  
>      bool match;
>  
>      if (IsOrigin()) {

It would be slightly better if this and other Matches* functions were written with Variant's match() support:

http://dxr.mozilla.org/mozilla-central/source/mfbt/Variant.h#399

Adding new cases would be caught at compile-time, rather than run-time.

Comment 10

2 years ago
Thanks for the feedback! 

It helps me to find out some problems in my patch! I will address them into my patch.

(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #8)
> @@ +225,5 @@
> >  
> >      bool match;
> >  
> >      if (IsOrigin()) {
> > +      match = GetPattern().Matches(GetOriginAttributes());
> 
> Boom! We die here because this->IsOrigin() is false. 
> We should use aOther.GetPattern() for matching:
>       match = aOther.GetPattern().Matches(GetOriginAttributes());

Oh, I should find it out before sending request :(

> @@ +312,5 @@
> > +  {
> > +    mOriginScope.emplace(mozilla::OriginAttributesPattern());
> > +    mozilla::OriginAttributesPattern pattern =
> > +      mOriginScope->as<mozilla::OriginAttributesPattern>();
> > +    MOZ_ALWAYS_TRUE(pattern.Init(aJSONPattern));
> 
> I think we can just call SetFromJSONPattern(aJSONPatten) here.

I will apply it to the other ctors if they can as well.

Comment 11

2 years ago
Thanks for the feedback, too!

(In reply to Nathan Froyd [:froydnj] from comment #9)
> ::: dom/quota/OriginScope.h
> @@ +247,4 @@
> >  
> >      bool match;
> >  
> >      if (IsOrigin()) {
> 
> It would be slightly better if this and other Matches* functions were
> written with Variant's match() support:
> 
> http://dxr.mozilla.org/mozilla-central/source/mfbt/Variant.h#399
> 
> Adding new cases would be caught at compile-time, rather than run-time.

I was thinking that it might be a problem (makes all the member functions become static), but it turn out it's not a problem. 
So, I will try to implement that!

Comment 12

2 years ago
Created attachment 8825630 [details] [diff] [review]
Bug-1324836-v2- Make QriginScope use mozilla::Variant.

- Address the comment!
try : https://treeherder.mozilla.org/#/jobs?repo=try&revision=63d803050379cb7f5b2dd0f20f3eea3fef3c9360
Attachment #8824900 - Attachment is obsolete: true

Comment 13

2 years ago
Comment on attachment 8825630 [details] [diff] [review]
Bug-1324836-v2- Make QriginScope use mozilla::Variant.

Hi Bevis,

I address comment 8 & 9, could you give me some feedback again? Thanks!
Attachment #8825630 - Flags: feedback?(btseng)
Created attachment 8825705 [details] [diff] [review]
Keep MatchesXxx non-static

Here is my WIP for your reference to Keep MatchXxx non-static for your reference.
In addition, please also double check if there original design of the |matches| function promise that A.matchesXxx(B) == B.matchesXxx(A).
Comment on attachment 8825630 [details] [diff] [review]
Bug-1324836-v2- Make QriginScope use mozilla::Variant.

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

It looks better, but I still suggest to keep the |Matches| methods non-static.
Please check the modified version of your patch attached in comment 13.

In addition, please also double check if there original design of the |matches| function promise that A.matchesXxx(B) == B.matchesXxx(A), if we modify the code in ActorsParent.cpp.
Attachment #8825630 - Flags: feedback?(btseng)

Comment 16

2 years ago
Created attachment 8826095 [details] [diff] [review]
Bug-1324836-v3- Make QriginScope use mozilla::Variant.

Hi Bevis,

Thanks for the feedback! Your suggestion for using non-static member function is great!

Basically, I address your comment in this patch, but I would like to keep using  |MatchesXXX(const XXXType& aXXX)| rather than aOther.  

For the case |A.matchesXxx(B) == B.matchesXxx(A)|, I believe the logic here is same as original code. Because, in each match, we return the same functions as results and pass with the same arguments to each of function.

Could you give me feedback again? Thanks!!
Attachment #8825630 - Attachment is obsolete: true
Attachment #8826095 - Flags: feedback?(btseng)
Comment on attachment 8826095 [details] [diff] [review]
Bug-1324836-v3- Make QriginScope use mozilla::Variant.

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

Looks great!
If you think aXxx (The Xxx represents the type) is important to you, then
I think aOtherXxx is still better for readability to prevent improper comparison accidentally.
Attachment #8826095 - Flags: feedback?(btseng) → feedback+

Comment 18

2 years ago
Created attachment 8826491 [details] [diff] [review]
Bug-1324836-v4- Make QriginScope use mozilla::Variant. f=bevis.

(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #17)
Thanks for the feedback! I learn a lot from them!
 
I find out that aOtherXXX is too long, so I address your previous comment which is using aOther/mOther to indicate OtherType.
Attachment #8826095 - Attachment is obsolete: true

Comment 19

2 years ago
Comment on attachment 8826491 [details] [diff] [review]
Bug-1324836-v4- Make QriginScope use mozilla::Variant. f=bevis.

Hi Jan,

I use Maybe<..> and Variant<..> to replace the original OriginScope. Besides, I move MatchesOrigin()/MatchesPattern()/MatchesPrefix() from public into private since the same logic come be done by just calling Match(..). 
Could you help me to review this patch? Thanks!

Hi Nathan,

Since you are the reporter and you've already give me some initial feedback, would you mind helping me to review this patch as well? Maybe review the part to using Maybe<> and Variant<> especially. Thanks!
Attachment #8826491 - Flags: review?(nfroyd)
Attachment #8826491 - Flags: review?(jvarga)
(Assignee)

Comment 20

2 years ago
Comment on attachment 8826491 [details] [diff] [review]
Bug-1324836-v4- Make QriginScope use mozilla::Variant. f=bevis.

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

I haven't checked the "matchers" yet, just some obvious stuff.

::: dom/quota/OriginScope.h
@@ +52,2 @@
>  
> +  mozilla::Maybe<VariantType> mOriginScope;

It's a bit strange for class OriginScope to have a member mOriginScope.

@@ -203,5 @@
>  
>    const nsACString&
>    GetOrigin() const
>    {
> -    MOZ_ASSERT(IsOrigin());

So we don't need this assertion anymore, here and in other similar getters/setters below ?

Comment 21

2 years ago
(In reply to Jan Varga [:janv] from comment #20)
> Comment on attachment 8826491 [details] [diff] [review]
> Bug-1324836-v4- Make QriginScope use mozilla::Variant. f=bevis.
> 
> Review of attachment 8826491 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I haven't checked the "matchers" yet, just some obvious stuff.
> 
> ::: dom/quota/OriginScope.h
> @@ +52,2 @@
> >  
> > +  mozilla::Maybe<VariantType> mOriginScope;
> 
> It's a bit strange for class OriginScope to have a member mOriginScope.

Hmm, I will try to find out better one!

> @@ -203,5 @@
> >  
> >    const nsACString&
> >    GetOrigin() const
> >    {
> > -    MOZ_ASSERT(IsOrigin());
> 
> So we don't need this assertion anymore, here and in other similar
> getters/setters below ?

Right, since it will hit the asserts in Variant<..>::as(type) if the type is wrong [1] [2]. Sorry for forgetting mention that!

[1] https://dxr.mozilla.org/mozilla-central/source/mfbt/Variant.h#578
[2] https://dxr.mozilla.org/mozilla-central/source/mfbt/Variant.h#587
(Assignee)

Comment 22

2 years ago
Does it hurt to catch the problem a bit earlier, especially when it's a debug-only code ?
Anyway, SetPrefix() still have the assertion ...
If you decide to remove these assertions, you should at least add a comment that Variant::as will catch it, but I think that MOZ_ASSERT(IsOrigin()) is just easier, because it behaves like a comment too.
(In reply to Jan Varga [:janv] from comment #22)
> Does it hurt to catch the problem a bit earlier, especially when it's a
> debug-only code ?
Oh, the MOZ_RELEASE_ASSERT is used in Variant::as() instead.
> Anyway, SetPrefix() still have the assertion ...
Sorry for not capturing this. :\
> If you decide to remove these assertions, you should at least add a comment
> that Variant::as will catch it, but I think that MOZ_ASSERT(IsOrigin()) is
> just easier, because it behaves like a comment too.
IMHO, it's Variant's design intention to move all this assertion into Variant::as(), so the Variant user don't have to worry about this anymore.
Otherwise, it's a punishment to |true| condition that same ASSERTION has to be check twice in debug build. :-(
(Assignee)

Comment 24

2 years ago
Extra paranoia in debug builds never hurts, we do it all over IDB implementation, but if you think that this will badly affect debug builds... ok :)
(Reporter)

Comment 25

2 years ago
Comment on attachment 8826491 [details] [diff] [review]
Bug-1324836-v4- Make QriginScope use mozilla::Variant. f=bevis.

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

Looks reasonable to me, but please let Jan have the final say in everything.  Thanks for doing this!

::: dom/quota/OriginScope.h
@@ +125,2 @@
>  
> +    mOriginScope.emplace(OriginAndAttributes(aOrigin));

I don't think you need the extra constructor here and in other emplace() calls; does the code not work correctly if you remove the extra constructor call?

@@ -203,5 @@
>  
>    const nsACString&
>    GetOrigin() const
>    {
> -    MOZ_ASSERT(IsOrigin());

I think we should keep these assertions, even with Variant::as supposedly doing release-mode assertion checks.  The compiler can probably remove the redundant checks when doing debug + optimize builds, and in a completely unoptimized debug build, doing multiple checks here is not going to be your performance problem. ;)

@@ +309,5 @@
> +      bool match(const nsACString& aPrefix) {
> +        // The match will be always true here because any origin attributes
> +        // pattern overlaps any origin prefix (an origin prefix targets all
> +        // origin attributes).
> +        return true; }

Nit: closing brace on its own line, please.
Attachment #8826491 - Flags: review?(nfroyd) → review+

Comment 26

2 years ago
(In reply to Nathan Froyd [:froydnj] from comment #25)
Thanks for the review and feedback!

> Comment on attachment 8826491 [details] [diff] [review]
> Bug-1324836-v4- Make QriginScope use mozilla::Variant. f=bevis.
> 
> Review of attachment 8826491 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks reasonable to me, but please let Jan have the final say in everything.
> Thanks for doing this!
> 
> ::: dom/quota/OriginScope.h
> @@ +125,2 @@
> >  
> > +    mOriginScope.emplace(OriginAndAttributes(aOrigin));
> 
> I don't think you need the extra constructor here and in other emplace()
> calls; does the code not work correctly if you remove the extra constructor
> call?

Do you mean the OriginAndAttributes(aOrigin)? If the answer is yes, we want to distinguish the prefix(nsCString) and origin(OriginAndAttributes->nsCString).

> @@ -203,5 @@
> >  
> >    const nsACString&
> >    GetOrigin() const
> >    {
> > -    MOZ_ASSERT(IsOrigin());
> 
> I think we should keep these assertions, even with Variant::as supposedly
> doing release-mode assertion checks.  The compiler can probably remove the
> redundant checks when doing debug + optimize builds, and in a completely
> unoptimized debug build, doing multiple checks here is not going to be your
> performance problem. ;)

Ok, I will add them back since both :jan and you suggest that.

Comment 27

2 years ago
Created attachment 8827065 [details] [diff] [review]
Bug-1324836-v5- Make QriginScope use mozilla::Variant. f=bevis.

Update the patch and address the comment so far.
- Add the assertions in getter funcitons and setter funcitons back.
- Rename mOriginScope to mData.
- Fix the nits.

Comment 28

2 years ago
(In reply to Tom Tung [:tt] from comment #26)
> (In reply to Nathan Froyd [:froydnj] from comment #25)
> Thanks for the review and feedback!
> 
> > Comment on attachment 8826491 [details] [diff] [review]
> > Bug-1324836-v4- Make QriginScope use mozilla::Variant. f=bevis.
> > 
> > Review of attachment 8826491 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Looks reasonable to me, but please let Jan have the final say in everything.
> > Thanks for doing this!
> > 
> > ::: dom/quota/OriginScope.h
> > @@ +125,2 @@
> > >  
> > > +    mOriginScope.emplace(OriginAndAttributes(aOrigin));
> > 
> > I don't think you need the extra constructor here and in other emplace()
> > calls; does the code not work correctly if you remove the extra constructor
> > call?
> 
> Do you mean the OriginAndAttributes(aOrigin)? If the answer is yes, we want
> to distinguish the prefix(nsCString) and
> origin(OriginAndAttributes->nsCString).

I find out I cant remove one for SetFromPattern() since they both are OriginAttributesPattern.

Comment 29

2 years ago
There is a typo in the previous comment :P

(In reply to Tom Tung [:tt] from comment #26)
> > I don't think you need the extra constructor here and in other emplace()
> > calls; does the code not work correctly if you remove the extra constructor
> > call?
> 
> Do you mean the OriginAndAttributes(aOrigin)? If the answer is yes, we want
> to distinguish the prefix(nsCString) and
> origin(OriginAndAttributes->nsCString).

I find out I can remove one for SetFromPattern() since they both are OriginAttributesPattern.

Comment 30

2 years ago
Created attachment 8827815 [details] [diff] [review]
Bug-1324836-v5.1- Make QriginScope use mozilla::Variant. f=bevis. r=froydnj.

Update patch per comment#29
Attachment #8827065 - Attachment is obsolete: true
(Assignee)

Comment 31

2 years ago
btw, here's |sizeof(OriginScope)| comparison:
16 w/o changes
136 patch applied
128 Maybe<...> removed

So, even null OriginScope will take 128 bytes.
Anyway, this comparison is not fair since current implementation allocates additional structures dynamically and it needs to keep pointers to them.

Anyway, Maybe<...> can be removed I believe, I have a patch for that, needs more testing ...
(Assignee)

Comment 32

2 years ago
I think it would be great to add tests for OriginScope. It has to be written in C++ I guess.
We are doing quite big changes here and I don't feel comfortable with giving final r+ w/o having some tests.
For example, the test should check (pseudo code):
OriginScope::FromOrigin("http+++www.mozilla.org^appId=1007") matches OriginScope::FromPattern("appId=1007");

Tom, could you write such a test ?
Thanks.

Comment 33

2 years ago
(In reply to Jan Varga [:janv] from comment #32)
> I think it would be great to add tests for OriginScope. It has to be written
> in C++ I guess.
> We are doing quite big changes here and I don't feel comfortable with giving
> final r+ w/o having some tests.
> For example, the test should check (pseudo code):
> OriginScope::FromOrigin("http+++www.mozilla.org^appId=1007") matches
> OriginScope::FromPattern("appId=1007");
> 
> Tom, could you write such a test ?
> Thanks.

Sure, but I've never written c++ test to test class functionality in gecko before. Do you mean Gtest? 
I'll work on it and provide a patch for tests!
(Assignee)

Comment 34

2 years ago
Yes, a GTest.
(Assignee)

Comment 35

2 years ago
Created attachment 8833742 [details] [diff] [review]
patch
(Assignee)

Comment 36

2 years ago
Created attachment 8833743 [details] [diff] [review]
interdiff
(Assignee)

Comment 37

2 years ago
Created attachment 8833744 [details]
OriginScope.h
(Assignee)

Updated

2 years ago
Attachment #8826491 - Flags: review?(jvarga)
(Assignee)

Comment 38

2 years ago
OriginAttributes and OriginAttributesPattern are quite big structs so I decided to allocate them dynamically. I also replaced Maybe with a Null variant. OriginScope now takes 32 bytes (up 16 bytes from current m-c, but down 104 bytes from v5.1 patch). It's not exact comparison, but the pattern type is not used much and it takes over 100 bytes, so I think it makes sense to allocate it dynamically. There are also some other fixes and improvements.

I'm going to ask :asuth for review.

Tom, you can work on the test in the meantime.

Thanks!
(Assignee)

Comment 39

2 years ago
Comment on attachment 8833742 [details] [diff] [review]
patch

Andrew, there's also an interdiff patch and whole OriginScope.h attached. I recommend looking at the whole file.
Attachment #8833742 - Flags: review?(bugmail)

Comment 40

2 years ago
Created attachment 8833917 [details] [diff] [review]
[WIP] Bug-1324836-P2- gtest for OriginScope.

Init patch for gtest to test OriginScope::Matches()
Comment on attachment 8833742 [details] [diff] [review]
patch

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

::: dom/quota/OriginScope.h
@@ +51,2 @@
>  
> +    Origin(const Origin& aOther)

This copy-constructor will suppress the implicit move constructor, per http://en.cppreference.com/w/cpp/language/move_constructor and it's likely the lack of move-support is obscured by the fact that implicitly converts T&& to T&.

Since Move() is explicitly used throughout, and UniquePtr does not have a copy constructor (meaning you can't use the default/implicit copy constructor), I think the right thing to do is:
  Origin(Origin&& aOther) == default

@@ +90,5 @@
> +    {
> +      MOZ_ALWAYS_TRUE(mPattern->Init(aJSONPattern));
> +    }
> +
> +    Pattern(const Pattern& aOther)

Same deal with suppressed implicit move, suggest adding:
  Pattern(Pattern&& AOther) = default;
Comment on attachment 8833742 [details] [diff] [review]
patch

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

::: dom/quota/OriginScope.h
@@ +226,5 @@
> +    Origin& origin = mData.as<Origin>();
> +
> +    origin.mOrigin = aOrigin;
> +
> +    origin.InitAttributes();

Acknowledging that I see the call to InitAttributes() was added and that it's more correct than what was there before, but it's not a big deal because it does not change QM behavior.  The affected code does an Origin-matches-Origin check which only compares the origins, not the attributes.  It was bad they were stale and this is better, but it didn't matter.

== Details, no need to read:

The existing use of SetOrigin in QM is:
  nsCString originSanitized(originScope.GetOrigin());
  SanitizeOriginString(originSanitized);
  originScope.SetOrigin(originSanitized);
Where SanitizeOriginString replaces kReplaceChars with +'s:
  QuotaManager::kReplaceChars[] = CONTROL_CHARACTERS "/:*?\"<>|\\";
With the mainly observable effect of "http://example.com" becoming "http+++example.com".

Because OriginAttributes cares about '^', '=', and '&' and these are left untouched, the only thing that matters is string attributes which could have contents that could get mangled.  OriginAttributesDictionary has string members mAddonId and mFirstPartyDomain, all the rest are bool or uint32_t.  It sorta looks like mAddonId should be a FS-safe UUID or a "foo@example.org" value, and so should never have been sanitized.  But at least a few tests indicate firstPartyDomain includes the "https://" bit, so they would likely have been affected if matching against a pattern.  (But no one did that.)
Attachment #8833742 - Flags: review?(bugmail) → review+

Comment 43

2 years ago
Created attachment 8834283 [details] [diff] [review]
Bug-1324836-P2 - v0 - Gtest for OriginScope.
Attachment #8833917 - Attachment is obsolete: true

Comment 44

2 years ago
Created attachment 8834296 [details] [diff] [review]
Bug-1324836-P2 - v0.1 - Gtest for OriginScope.

Hi Jan,

I wrote a gtest to test OriginScope. Could you help me to review it? However, there are two things that I'm not pretty sure about that. I listed them below inline.

The first thing is that I notice there is a warning [1](file not found) when calling OriginAttributesPatternDictionary::Init(). This warning doesn't effect on the result of test so I'm not sure whether it is matter.  

Besides, could you take more care on prefix part? I'm not sure about the test case for prefix is correct since I'm not sure exactly about the definition of origin prefix. Does it represent to the scheme of an origin? Or just "prefix" of origin? (like: http://moz, http://mozilla ?)

[1] http://searchfox.org/mozilla-central/source/extensions/cookie/nsPermissionManager.cpp#2652
Attachment #8834283 - Attachment is obsolete: true
Attachment #8834296 - Flags: review?(jvarga)
(Assignee)

Comment 45

2 years ago
The prefix was added for clearing entire sites. For example, if we have:
http://www.mozilla.org
http://www.mozilla.org^userContextId=1
http://www.mozilla.org^userContextId=2
then we create OriginScope with "http://www.mozilla.org" prefix to clear all these origin directories

We don't care about variations like http://moz, http://mozilla, etc. right now.

Comment 46

2 years ago
(In reply to Jan Varga [:janv] from comment #45)
> The prefix was added for clearing entire sites. For example, if we have:
> http://www.mozilla.org
> http://www.mozilla.org^userContextId=1
> http://www.mozilla.org^userContextId=2
> then we create OriginScope with "http://www.mozilla.org" prefix to clear all
> these origin directories
> 
> We don't care about variations like http://moz, http://mozilla, etc. right
> now.

Oh, I see! Thanks for the explanation! I will update test later!

Comment 47

2 years ago
Created attachment 8834343 [details] [diff] [review]
Bug-1324836-P2 - v1 - Gtest for OriginScope.

Update patch for changing test case about prefix and tests for testing matches() function.
Attachment #8834296 - Attachment is obsolete: true
Attachment #8834296 - Flags: review?(jvarga)
Attachment #8834343 - Flags: review?(jvarga)

Comment 48

2 years ago
Is this work still active or should we unassign and open it up for other contributors?
Flags: needinfo?(jvarga)
(Assignee)

Comment 49

2 years ago
(In reply to Eric Rahm [:erahm] from comment #48)
> Is this work still active or should we unassign and open it up for other
> contributors?

This is a low priority bug. The primary fix is finished/reviewed. It's the C++ test that needs to be verified by me.
I don't think we need other contributors here.
Flags: needinfo?(jvarga)

Updated

9 months ago
Assignee: ttung → nobody
Status: ASSIGNED → NEW

Comment 50

6 months ago
(In reply to Jan Varga [:janv] from comment #49)
> (In reply to Eric Rahm [:erahm] from comment #48)
> > Is this work still active or should we unassign and open it up for other
> > contributors?
> 
> This is a low priority bug. The primary fix is finished/reviewed. It's the
> C++ test that needs to be verified by me.
> I don't think we need other contributors here.

Okay lets clear the good first bug flag since this is basically done, but isn't assigned.
Keywords: good-first-bug
(Assignee)

Comment 51

2 months ago
I'm going to resurrect this.
(Assignee)

Updated

a month ago
Attachment #8825705 - Attachment is obsolete: true
(Assignee)

Updated

a month ago
Attachment #8826491 - Attachment is obsolete: true
(Assignee)

Updated

a month ago
Attachment #8827815 - Attachment is obsolete: true
(Assignee)

Comment 52

a month ago
Comment on attachment 8834343 [details] [diff] [review]
Bug-1324836-P2 - v1 - Gtest for OriginScope.

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

This looks too complicated. We need something more straightforward.
Attachment #8834343 - Flags: review?(jvarga)
(Assignee)

Updated

a month ago
Attachment #8834343 - Attachment is obsolete: true
(Assignee)

Updated

a month ago
Assignee: nobody → jvarga
Status: NEW → ASSIGNED
(Assignee)

Comment 53

a month ago
Created attachment 9007150 [details] [diff] [review]
rebased main patch
Attachment #9007150 - Flags: review+
(Assignee)

Comment 54

a month ago
Created attachment 9007151 [details] [diff] [review]
gtest
(Assignee)

Comment 55

a month ago
Comment on attachment 9007151 [details] [diff] [review]
gtest

Andrew, you r+'ed the main patch long time ago, but I somehow couldn't find time to review/update a new gtest for it.
I had to do something similar for new local storage recently, so I decided to finish this bug too.
Thanks.
Attachment #9007151 - Flags: review?(bugmail)
Attachment #9007151 - Flags: review?(bugmail) → review+

Comment 56

a month ago
Pushed by jvarga@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/35c9f4bc32ed
Part 1: Make OriginScope use mozilla::Variant; r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/898a443cbe3d
Part 2: Add a gtest for OriginScope testing; r=asuth

Comment 57

a month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/35c9f4bc32ed
https://hg.mozilla.org/mozilla-central/rev/898a443cbe3d
Status: ASSIGNED → RESOLVED
Last Resolved: a month ago
status-firefox64: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.