1229222 - add chromeutils for the creation of origin attributes with the correct default values

Status: RESOLVED FIXED

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

Description

[Dave Huseby [:huseby]]

There is a subset of callers to createCodePrincipal that specifically deal with user permissions, feeds, accounts, bookmarks, sync, and a few other corner cases where we are not going to isolation on the user context ID.  This requires us to create a new type of OriginAttribute (OA) subclass that has its own inherit functions for initialization that always slams the user context ID to 0.

This new type of OA needs to always be used in the managers for these settings/prefs/values and for the about: pages that implement the user interface for managing them.

Comment 1

[Dave Huseby [:huseby]]

What should we call this new OA?  Some ideas:

PrefOriginAttribute
PermissionOriginAttribute
AboutColonOriginAttribute
UserDataOriginAttribute

I'm not sure what would be best.  I'm still looking for a good name.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I think PermissionOriginAttributes would go with what we had in mind (i.e. something only used for reading/writing permissions)

Comment 3

[Dave Huseby [:huseby]]

Attachment: Bug_1229222.hgpatch

Adds PermissionOriginAttributes.

Comment 4

[Dave Huseby [:huseby]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a9e131d76e6d

Comment 5

[Tanvi Vyas[:tanvi]]

(In reply to Jonas Sicking (:sicking) from comment #2)
> I think PermissionOriginAttributes would go with what we had in mind (i.e.
> something only used for reading/writing permissions)

Sorry to bikeshed this.  But we are planning to use this for about:feeds and about:preferences too.  Hence, PermissionsOriginAttributes is more specific than our use cases.  But permissions are a "preference" so PreferencesOriginAttributes might be a better name.  Jonas?

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Permissions are managed by the nsIPermissionManager, no? Not preferences?

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Preferences aren't keyed on origins, uris, or origin-attributes, so I'm not sure how they play in here?

I also don't know how about:feeds work. What storage API does it use?

Comment 8

[Dave Huseby [:huseby]]

Attachment: Bug_1229222.hgpatch

Turns out that PermissionOriginAttributes is just a special case of PrincipalOriginAttributes so I adjusted the C++ accordingly.  This patch also covers fixing up the permissions manager and permissions so that they use PermissionOriginAttributes instead of PrincipalOriginAttributes.

Comment 9

[Dave Huseby [:huseby]]

So from my audit notes:

>extensions/cookie/nsPermissionManager.cpp
>  line 119
>    nsCOMPtr<nsIPrincipal> principal = mozilla::BasePrincipal::CreateCodebasePrincipal(uri, attrs);
> 
>  line 130
>    nsCOMPtr<nsIPrincipal> principal = mozilla::BasePrincipal::CreateCodebasePrincipal(aURI, attrs);
> 
>  line 141
>    nsCOMPtr<nsIPrincipal> principal = mozilla::BasePrincipal::CreateCodebasePrincipal(aURI, attrs);
> 
>  line 2172
>    nsCOMPtr<nsIPrincipal> principal = mozilla::BasePrincipal::CreateCodebasePrincipal(newURI, attrs);
> 
> NOTE: fixed by switching to PermissionOriginAttributes
> 
> extensions/cookie/nsPermission.cpp
>  line 171
>    nsCOMPtr<nsIPrincipal> principal = mozilla::BasePrincipal::CreateCodebasePrincipal(aURI, attrs);
> 
> NOTE: fixed by switching to PermissionOriginAttributes

This patch covers these parts of my audit.

Comment 10

[Dave Huseby [:huseby]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=96a6498d268a

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

So I was thinking about this one. It might be simpler to not add a new struct-type and instead simply make the permissionmanager code ignore the contextId in the PrincipalOriginAttribute that it is passed.

Possibly we could have done this for NeckoOriginAttributes as well...

If it later turns out that a new struct is better we can always implement that then. This way we get to compare both approaches.

Comment 12

[Dave Huseby [:huseby]]

Attachment: Bug_1229222.hgpatch

Comment 13

[Dave Huseby [:huseby]]

Attachment: add PermissionOriginAttribute class

small patch that just adds the PermissionOriginAttribute class.

Comment 14

[Dave Huseby [:huseby]]

Attachment: fix up permissions and permission manager

patch that *just* fixes the permission and permission manager.

Comment 15

[Dave Huseby [:huseby]]

(In reply to Jonas Sicking (:sicking) from comment #11)
> So I was thinking about this one. It might be simpler to not add a new
> struct-type and instead simply make the permissionmanager code ignore the
> contextId in the PrincipalOriginAttribute that it is passed.

What do you mean by struct-type?  AFAICT, the only reason OriginAttributes are a class is so they derive from the JS::Dictionary for easier interop with JS.

Personally, IMO the Inherit*() functions are clunky.  I would have just created explicit copy constructors for the desired conversions like so:

> class PrincipalOriginAttribute {
>     explicit PrincipalOriginAttributes(const NeckoOriginAttributes& rhs) { /* copy the attr's we want */ }
> }

I understand that calling Inherit* is more explicit, but I think C++ conversion copy constructors are more C++-ish.  But my yaks need shaving! :)

Comment 16

[Bobby Holley (:bholley)]

The explicitness of Inherit is pretty important, IMO - it makes the fact that a conversion is taking place more obvious to the reader, and also makes it clear _why_ that conversion needs to happen (i.e. InheritFromDocToChildDocShell). At one point we had several conversion routines with the same |this| and argument types, in which case separate names is the only way to differentiate them.

Comment 17

[Dave Huseby [:huseby]]

This bug now is limited to just the new DefaultContextOriginAttributes type and the associated utility functions.

Comment 18

[Dave Huseby [:huseby]]

(In reply to Bobby Holley (busy) from comment #16)
> The explicitness of Inherit is pretty important, IMO - it makes the fact
> that a conversion is taking place more obvious to the reader, and also makes
> it clear _why_ that conversion needs to happen (i.e.
> InheritFromDocToChildDocShell). At one point we had several conversion
> routines with the same |this| and argument types, in which case separate
> names is the only way to differentiate them.

The problem with the Inherit* design is that the underlying type of all origin attributes is a JS::Dictionary which makes them automatically interchangeable.  Also, none of the Inherit* functions are exposed to Javascript.  When we ruled out { userContextID = 0; } as a manual fixup solution, then we're back to solutions similar to the recently-deprecated getNoAppPrincipal().  Utility functions that create/convert special purpose origin attributes that can then be used to create a principal from Javascript code.

My patch for this bug creates a new DefaultContextOriginAttributes type and a ChromeUtils function that takes any origin attribute and returns me an origin attributes dictionary with the userContextID set to 0.  That's my solution for avoiding having to manually fix up the value of userContextID everywhere and it doesn't use any of the Inherit* functions.

If we had an nsIOriginAttribute interface that had functions like createNeckoOAFromDocShellOA() that I could call from Javascript, then maybe the inherit functions would work for all of the fixup I'm doing now?  Thoughts?

Comment 19

[Dave Huseby [:huseby]]

Attachment: Bug_1229222.patch

Comment 20

[Bobby Holley (:bholley)]

(In reply to Dave Huseby [:huseby] from comment #18)
> The problem with the Inherit* design is that the underlying type of all
> origin attributes is a JS::Dictionary which makes them automatically
> interchangeable.

In C++ or JS? They definitely shouldn't be interchangable in C++. But yes, this doesn't really help us in JS.

>  Also, none of the Inherit* functions are exposed to
> Javascript.

We can expose those methods on ChromeUtils as appropriate if that'd be helpful.

I'm not really in the loop enough to comment further, so I'll leave the rest between you and sicking.

Comment 21

[Dave Huseby [:huseby]]

Attachment: Bug_1229222.patch

fixed some naming issues, thanks Tanvi.

Comment 22

[Dave Huseby [:huseby]]

Attachment: Bug_1229222.patch

added definitions for PopulateFromSuffix and PopulateFromOrigin in the DefaultContextOriginAttributes class so that the mUserContextId gets forced to the default value after the population process happens.

Comment 23

[Dave Huseby [:huseby]]

Attachment: Bug_1229222.patch

Comment 24

[Andrea Marchesini [:baku]]

The latest version of this patch doesn't compile because
|mozilla::PrincipalOriginAttributes& mozilla::PrincipalOriginAttributes::operator=(const mozilla::dom::OriginAttributesDictionary&)| is not defined.

Comment 25

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The statement "for places where we don't want context separation" only refers to user context, right? Other types of contexts, like B2G apps, or Tor contexts, will likely want to separate permissions and other things into separate "jars".

Comment 26

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8704392 [details] [diff] [review]
Bug_1229222.patch

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

::: caps/BasePrincipal.h
@@ +138,5 @@
> +{
> +public:
> +  DefaultContextOriginAttributes()
> +  {
> +    mUserContextId = nsIScriptSecurityManager::DEFAULT_USER_CONTEXT_ID;

Why is this needed? Shouldn't the OriginAttributes baseclass default ctor set this?

@@ +145,5 @@
> +  DefaultContextOriginAttributes(uint32_t aAppId, bool aInBrowser)
> +  {
> +    mAppId = aAppId;
> +    mInBrowser = aInBrowser;
> +    mUserContextId = nsIScriptSecurityManager::DEFAULT_USER_CONTEXT_ID;

Is this ctor really needed? If so, why?

@@ +156,5 @@
> +
> +  DefaultContextOriginAttributes& operator=(const OriginAttributesDictionary& aOther)
> +  {
> +    PrincipalOriginAttributes::operator=(aOther);
> +    mUserContextId = nsIScriptSecurityManager::DEFAULT_USER_CONTEXT_ID;

In general I don't understand the design here. It feels both non-generic (special treatment to some attributes over others) and surprising (why does the assignment operator not copy all attributes but the copy ctor does?)

I guess I don't really understand how this struct is intended to be used.

Comment 27

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

To be clear, I'm not saying that a new struct is wrong. I just don't understand the current approach.

Comment 28

[Dave Huseby [:huseby]]

Attachment: Bug_1229222.patch

rebased.

Comment 29

[Dave Huseby [:huseby]]

Attachment: Bug_1229222_Tests.patch

tests.

Comment 30

[Dave Huseby [:huseby]]

Attachment: Bug_1229222.patch

Alright, after fixing up a bunch of patches, I think this is the minimal set of changes needed to accomplish what we want.

Comment 31

[Dave Huseby [:huseby]]

Attachment: Bug_1229222_Tests.patch

Full coverage tests for the changes.

Comment 32

[Dave Huseby [:huseby]]

Attachment: Bug_1229222_Tests.patch

full coverage tests for the changes.

Comment 33

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8713777 [details] [diff] [review]
Bug_1229222.patch

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

::: dom/base/ChromeUtils.cpp
@@ +98,5 @@
> +                                 const dom::OriginAttributesDictionary& aAttrs,
> +                                 dom::OriginAttributesDictionary& aNewAttrs)
> +{
> +  GenericOriginAttributes attrs(aAttrs);
> +  aNewAttrs = attrs;

Do you even need the temporary? Wouldn't simply

aNewAttrs = aAttrs

work?

@@ +135,5 @@
> +
> +/* static */ void
> +ChromeUtils::SetDefaultUserContextId(dom::GlobalObject& aGlobal,
> +                                 const dom::OriginAttributesDictionary& aAttrs,
> +                                 dom::OriginAttributesDictionary& aNewAttrs)

Why is this function needed? Why not just have the JS code do

myDict.userContextId = 0;

Or, if you want to initialize all non-set members to default values, and also set the userContextId to default value, then:

myDict = chromeUtils.createOriginAttributesFromDict(myDict);
myDict.userContextId = 0;

?

::: dom/webidl/ChromeUtils.webidl
@@ +75,5 @@
> +   *                          both, including defaults.
> +   */
> +  static OriginAttributesDictionary
> +  setOriginAttributes(optional OriginAttributesDictionary originAttrs,
> +                      optional OriginAttributesPatternDictionary pattern);

I don't understand what this function is intended to do, or when it's supposed to be used.

It appears that the documentation above has typos in it because there is no setAttrs parameter, and there is no description of the 'pattern' parameter.

Also, OriginAttributesPatternDictionary was intended to be a way to test if given OA-struct matches a given wild-card enabled pattern. It was not meant as a way to initialize OA-structs.

I'm curious to understand what you are doing that needs this function.

Comment 34

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8713777 [details] [diff] [review]
Bug_1229222.patch

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

For what it's worth, the review comments here and in bug 1233904 is holding up my other UserContextId reviews as well since those patches seem very similar.

Comment 35

[Dave Huseby [:huseby]]

(In reply to Jonas Sicking (:sicking) from comment #33)
> Comment on attachment 8713777 [details] [diff] [review]
> Bug_1229222.patch
> 
> Review of attachment 8713777 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/ChromeUtils.cpp
> @@ +98,5 @@
> > +                                 const dom::OriginAttributesDictionary& aAttrs,
> > +                                 dom::OriginAttributesDictionary& aNewAttrs)
> > +{
> > +  GenericOriginAttributes attrs(aAttrs);
> > +  aNewAttrs = attrs;
> 
> Do you even need the temporary? Wouldn't simply
> 
> aNewAttrs = aAttrs
> 
> work?

I think you're right.  That's a vestige of when we were creating a DefaultContextOriginAttributes instance from the OriginAttributesDictionary.  Let me test it to make sure.
 
> @@ +135,5 @@
> > +
> > +/* static */ void
> > +ChromeUtils::SetDefaultUserContextId(dom::GlobalObject& aGlobal,
> > +                                 const dom::OriginAttributesDictionary& aAttrs,
> > +                                 dom::OriginAttributesDictionary& aNewAttrs)
> 
> Why is this function needed? Why not just have the JS code do
> 
> myDict.userContextId = 0;
> 
> Or, if you want to initialize all non-set members to default values, and
> also set the userContextId to default value, then:
> 
> myDict = chromeUtils.createOriginAttributesFromDict(myDict);
> myDict.userContextId = 0;
> 
> ?

I did it this way for two reasons:

#1 We wanted to avoid setting origin attributes by hand in this way because of maintenance reasons.  If we later decide to choose a different default value, change the name of the attribute, and/or make the default some result of a function, I don't want to have to go back through the code grep'ing for assignments like this.  With it this way, we would only have to change the body of this function.

#2 This also gives us a chance to make sure that all of the default attributes are defined and have default values as well.  By forcing a JS -> C++ -> JS transition, we will end up with a JS dictionary will all of the members.

> ::: dom/webidl/ChromeUtils.webidl
> @@ +75,5 @@
> > +   *                          both, including defaults.
> > +   */
> > +  static OriginAttributesDictionary
> > +  setOriginAttributes(optional OriginAttributesDictionary originAttrs,
> > +                      optional OriginAttributesPatternDictionary pattern);
> 
> I don't understand what this function is intended to do, or when it's
> supposed to be used.
> 
> It appears that the documentation above has typos in it because there is no
> setAttrs parameter, and there is no description of the 'pattern' parameter.
> 
> Also, OriginAttributesPatternDictionary was intended to be a way to test if
> given OA-struct matches a given wild-card enabled pattern. It was not meant
> as a way to initialize OA-structs.
> 
> I'm curious to understand what you are doing that needs this function.

You're right about the typo, good catch.  I'll fix it.  The OriginAttributesPatternDictionary is handy for testing if an attributes was explicitly set and not just the default value.  That makes it possible to do:

> attrs = ChromeUtils.setAttributes(attrs, {userContextId: 0, inBrowser: true});

Without knowing if a member is manually set, you wind up overwriting all members of attrs and not just the ones defined in the second attribute.

Comment 36

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> #1 We wanted to avoid setting origin attributes by hand in this way because
> of maintenance reasons.  If we later decide to choose a different default
> value, change the name of the attribute, and/or make the default some result
> of a function, I don't want to have to go back through the code grep'ing for
> assignments like this.  With it this way, we would only have to change the
> body of this function.
> 
> #2 This also gives us a chance to make sure that all of the default
> attributes are defined and have default values as well.  By forcing a JS ->
> C++ -> JS transition, we will end up with a JS dictionary will all of the
> members.

Why not

delete myDict.userContextId;
myDict = chromeUtils.createOriginAttributesFromDict(myDict);

or

myDict.userContextId = undefined;
myDict = chromeUtils.createOriginAttributesFromDict(myDict);

> > attrs = ChromeUtils.setAttributes(attrs, {userContextId: 0, inBrowser: true});
> 
> Without knowing if a member is manually set, you wind up overwriting all
> members of attrs and not just the ones defined in the second attribute.

Why not

attrs.userContextId = 0;
attrs.inBrowser = true;
attrs = chromeUtils.createOriginAttributesFromDict(attrs);

(where the latter is only needed if you want to make sure that all attributes have explicit default values)

Comment 37

[Dave Huseby [:huseby]]

(In reply to Jonas Sicking (:sicking) from comment #36)
> > #1 We wanted to avoid setting origin attributes by hand in this way because
> > of maintenance reasons.  If we later decide to choose a different default
> > value, change the name of the attribute, and/or make the default some result
> > of a function, I don't want to have to go back through the code grep'ing for
> > assignments like this.  With it this way, we would only have to change the
> > body of this function.
> > 
> > #2 This also gives us a chance to make sure that all of the default
> > attributes are defined and have default values as well.  By forcing a JS ->
> > C++ -> JS transition, we will end up with a JS dictionary will all of the
> > members.
> 
> Why not
> 
> delete myDict.userContextId;
> myDict = chromeUtils.createOriginAttributesFromDict(myDict);
> 
> or
> 
> myDict.userContextId = undefined;
> myDict = chromeUtils.createOriginAttributesFromDict(myDict);

You're referencing the attributes by name in those cases.  Those would work, but it makes the code more brittle.

> 
> > > attrs = ChromeUtils.setAttributes(attrs, {userContextId: 0, inBrowser: true});
> > 
> > Without knowing if a member is manually set, you wind up overwriting all
> > members of attrs and not just the ones defined in the second attribute.
> 
> Why not
> 
> attrs.userContextId = 0;
> attrs.inBrowser = true;
> attrs = chromeUtils.createOriginAttributesFromDict(attrs);
> 
> (where the latter is only needed if you want to make sure that all
> attributes have explicit default values)

This works too and I can't claim it is more brittle because I referenced the attributes by name in my example too.

Comment 38

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

You are referencing userContextId just as much in ChromeUtils.setDefaultUserContextId(...)

If we decide to change the name of userContextId, or remove it, in either way we'd need to search for references to setDefaultUserContextId/userContextId and figure out how to change the code appropriately.

Comment 39

[Dave Huseby [:huseby]]

(In reply to Jonas Sicking PTO until Feb 16th (:sicking) from comment #38)
> You are referencing userContextId just as much in
> ChromeUtils.setDefaultUserContextId(...)
> 
> If we decide to change the name of userContextId, or remove it, in either
> way we'd need to search for references to
> setDefaultUserContextId/userContextId and figure out how to change the code
> appropriately.

Fair enough.  So do you want me to get rid of setDefaultUserContextId and just use direct assignments?

Comment 40

[Dave Huseby [:huseby]]

(In reply to Jonas Sicking PTO until Feb 16th (:sicking) from comment #38)
> You are referencing userContextId just as much in
> ChromeUtils.setDefaultUserContextId(...)
> 
> If we decide to change the name of userContextId, or remove it, in either
> way we'd need to search for references to
> setDefaultUserContextId/userContextId and figure out how to change the code
> appropriately.

You're right about the name.  But I did it this way so that the C++ could refer to nsIScriptSecurityManager::DEFAULT_USER_CONTEXT_ID.  If all of our code calls this function when overwriting the user context id with the default, then if we ever change the default, we only have to change that constant in nsIScriptSecurityManager.

Comment 41

[Dave Huseby [:huseby]]

Attachment: Bug_1229222.patch

This patch reflects the API changes we agreed to.

Comment 42

[Dave Huseby [:huseby]]

Attachment: Bug_1229222_Tests.patch

updated tests for the new API.

Comment 43

[Tanvi Vyas[:tanvi]]

Comment on attachment 8720422 [details] [diff] [review]
Bug_1229222.patch

Thanks Dave for the updated patches!

>diff --git a/dom/webidl/ChromeUtils.webidl b/dom/webidl/ChromeUtils.webidl
>index 623b5cd..b067abb 100644
>--- a/dom/webidl/ChromeUtils.webidl
>+++ b/dom/webidl/ChromeUtils.webidl
>@@ -25,23 +25,48 @@ interface ChromeUtils : ThreadSafeChromeUtils {
>    * @param originAttrs       The originAttributes under consideration.
>    * @param pattern           The pattern to use for matching.
>    */
>   static boolean
>   originAttributesMatchPattern(optional OriginAttributesDictionary originAttrs,
>                                optional OriginAttributesPatternDictionary pattern);
> 
>   /**
>-   * Returns an OriginAttributes dictionary using the origin URI but forcing
>-   * the passed userContextId.
>+   * Returns an OriginAttributesDictionary with all default members specified
>+   * with default values.
What do you mean by default members specified with default values?  Do you mean all members specified with default values?
>+   *
>+   * @returns                 An OriginAttributesDictionary populated with the
>+   *                          default members with default values.
>+   */
>+   static OriginAttributesDictionary
>+   createDefaultOriginAttributes();


>+
>+  /**
>+   * Returns an OriginAttributesDictionary created from |origin| with default
>+   * members specified with default values.
>+   *
>+   * @param origin            The origin URI to create from.
>+   * @returns                 An OriginAttributesDictionary with values from
>+   *                          the origin suffix and unspecified default members
>+   *                          with default values.
>    */
>   [Throws]
>   static OriginAttributesDictionary
>-  createOriginAttributesWithUserContextId(DOMString origin,
>-                                          unsigned long userContextId);
>+  createOriginAttributesFromOrigin(DOMString origin);
>+
For this one, we are populating the origin attributes with things taken from the passed in origin.  Any unspecified values will get their default values.  So not sure if "default members specified with default values" is correct here.  Insetad maybe it should be "unspecified members are given default values"

>+  /**
>+   * Returns an OriginAttributesDictionary that is a copy of |originAttrs| with
>+   * any uspecified members added with default values.
unspecified
>+   *
>+   * @param originAttrs       The origin attributes to copy.
>+   * @returns                 An OriginAttributesDictionary copy of |originAttrs|
>+   *                          and default members specified with default values.
>+   */
>+  static OriginAttributesDictionary
>+  createOriginAttributesFromDict(optional OriginAttributesDictionary originAttrs);
> };
>

Comment 44

[Dave Huseby [:huseby]]

woohoo!!! \o/

Comment 45

[Tanvi Vyas[:tanvi]]

Hi Dave,

Please fix the "uspecified" typo referenced in comment 43, along with the other comments.  Removing checkin-needed for now.  Thanks!

Comment 46

[Dave Huseby [:huseby]]

Attachment: Bug_1229222.patch

Reworded descriptions and fixed typo.  Thanks Tanvi!

Comment 47

[Dave Huseby [:huseby]]

Tanvi,

If you approve, please flag it for checkin-needed.

Thanks!

Comment 48

[Tanvi Vyas[:tanvi]]

Thanks Dave!

Comment 49

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/736daf4b4a56
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4b8bb93313b

Comment 50

[Carsten Book [:Tomcat]]

backed this out in https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=d00d189cd28b since test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=22016049&repo=mozilla-inbound when this changes landed

Comment 51

[Dave Huseby [:huseby]]

Attachment: Bug_1229222.patch

r+ already by :sicking, backed out due to test failure.  this fixes the failure.

try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0fb3ba6513ad

Comment 52

[Dave Huseby [:huseby]]

Attachment: Bug_1229222_Tests.patch

r+ already by :sicking, backed out due to test failures. this fixes those failures.

try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0fb3ba6513ad

Comment 53

[Dave Huseby [:huseby]]

the push results are all green.  there's 5 oranges that don't look related.  setting for checkin-needed.

Comment 54

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6d7b620ac5ed
https://hg.mozilla.org/integration/mozilla-inbound/rev/13f5670eac32

Comment 55

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

https://hg.mozilla.org/mozilla-central/rev/6d7b620ac5ed
https://hg.mozilla.org/mozilla-central/rev/13f5670eac32

Comment 56

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/b2g-inbound/rev/c4207d6e57d0

Comment 57

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Reopened so I can track the landing (hope this is ok, correct me if this is wrong).

Comment 58

[Tanvi Vyas[:tanvi]]

(In reply to Paul Theriault [:pauljt] from comment #57)
> Reopened so I can track the landing (hope this is ok, correct me if this is
> wrong).

The patch landed to central on March 1st.  I'm not sure what the b2g-inbound backout in comment 56 is about.  Or if it is supposed to be relanded on b2g-inbound.

Comment 59

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to Tanvi Vyas [:tanvi] from comment #58)
> (In reply to Paul Theriault [:pauljt] from comment #57)
> > Reopened so I can track the landing (hope this is ok, correct me if this is
> > wrong).
> 
> The patch landed to central on March 1st.  I'm not sure what the b2g-inbound
> backout in comment 56 is about.  Or if it is supposed to be relanded on
> b2g-inbound.

Ah ok, you are right [1], I thought the backout was from moz central.

[1] https://hg.mozilla.org/mozilla-central/log/tip/browser/components/sessionstore/SessionStorage.jsm

Comment 60

[Tanvi Vyas[:tanvi]]

(In reply to Paul Theriault [:pauljt] from comment #59)
> (In reply to Tanvi Vyas [:tanvi] from comment #58)
> > (In reply to Paul Theriault [:pauljt] from comment #57)
> > > Reopened so I can track the landing (hope this is ok, correct me if this is
> > > wrong).
> > 
> > The patch landed to central on March 1st.  I'm not sure what the b2g-inbound
> > backout in comment 56 is about.  Or if it is supposed to be relanded on
> > b2g-inbound.
> 
> Ah ok, you are right [1], I thought the backout was from moz central.
> 
> [1]
> https://hg.mozilla.org/mozilla-central/log/tip/browser/components/
> sessionstore/SessionStorage.jsm

Do we need to reland in b2g-inbound?  Or is it okay if b2g doesn't have this code?  Is b2g code all being deprecated?

Comment 61

[Paul Theriault [:pauljt] (no longer reading bugmail)]

> Do we need to reland in b2g-inbound?  Or is it okay if b2g doesn't have this
> code?  Is b2g code all being deprecated?

No we don't - b2g is working in Pine now, which takes code from mozilla central. I double checked and this code is in Pine:

https://hg.mozilla.org/projects/pine/rev/6d7b620ac5ed