1233803 - Get rid of of sessionHistory CPOW sent up via WebNavigation:SetHistory message

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

We send a CPOW for sync access to the session history object. We shouldn't do that.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1233803 - Outlaw usage of sessionHistory CPOW in browser code. r?felipe

Review commit: https://reviewboard.mozilla.org/r/29461/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/29461/

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1233803 - Add RemoteWebNavigation sessionHistory shim for addons. r?krizsa

Review commit: https://reviewboard.mozilla.org/r/29463/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/29463/

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8703799 [details]
MozReview Request: Bug 1233803 - Outlaw usage of sessionHistory CPOW in browser code. r?felipe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29461/diff/1-2/

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8703800 [details]
MozReview Request: Bug 1233803 - Add RemoteWebNavigation sessionHistory shim for addons. r?krizsa

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29463/diff/1-2/

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1233803 - Register RemoteWebNavigation as a standard js-implemented XPCOM component. r?felipe

Review commit: https://reviewboard.mozilla.org/r/29885/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/29885/

Comment 6

[Gabor Krizsanits (INACTIVE)]

Mike needs an interposition for webNavigation, problem is that in the e10s case we have RemoteWebNavigation implemented in js (http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/RemoteWebNavigation.jsm#30)

We skip interposition for vanilla JS objects as an optimization: http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/wrappers/AddonWrapper.cpp#33

Should we 1) remove the optimization 2) add another case if the object has a QueryInterface method or do something else?

Comment 7

[Gabor Krizsanits (INACTIVE)]

 It's worth to mention that the object we are trying to do interposition on, is special in a sense that the WrapperFactory already detects that it has to apply "double-wrapping" on it. So I'm refreshing my memories about how to do that and try to use the same machinery. That way we could keep the optimization and extend the condition to include JS implemented XPCOM objects too. That would be the cleanest and sanest approach I could come up with.

Comment 8

[Gabor Krizsanits (INACTIVE)]

Bobby, I kind of assumed that for double wrapped objects the WN check here: http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/wrappers/AddonWrapper.cpp#34 would return true. But it does not seem to be the case. Does the UncheckedUnwrap call unwrap double wrapped objects completely? I would like to detect if the target is a JS implemented XPCOM object because I want to include these objects into the interposition. Do you know what is the best way to do that?

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Note that I'm blocked on this M8 until I can figure out how to shim this thing, or unless I can get a compelling argument not to shim it (it'd need to be pretty compelling, since a quick mxr shows a number of add-ons accessing webNavigation.sessionHistory)

Comment 10

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #8)
> Bobby, I kind of assumed that for double wrapped objects the WN check here:
> http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/wrappers/
> AddonWrapper.cpp#34 would return true.

I would expect IS_WN_CLASS to return true for a double-wrapped object, so !IS_WN_CLASS would be false, causing us not to take the early |return true| guarded on that conditional.

> But it does not seem to be the case.

Clarify?

> Does the UncheckedUnwrap call unwrap double wrapped objects completely?

It does not. UncheckedUnwrap unwraps JS wrappers. "Double-Wrapped Objects" should really be called "Double-Reflected Objects", and aren't related to the UncheckedUnwrap stuff.

> I
> would like to detect if the target is a JS implemented XPCOM object because
> I want to include these objects into the interposition. Do you know what is
> the best way to do that?

QI to nsIXPConnectWrappedJS. See UnwrapNativeCPOW for an example.

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Does that give you the information necessary for me to shim the nsIWebNavigation implementation in RemoteWebNavigation?

Comment 12

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (busy) from comment #10)
> > But it does not seem to be the case.
> 
> Clarify?

I was afraid you are going to say this... then my memories weren't half as bad as I hoped. I will debug this then and see why we don't get here what we both expect. One option is that this jsm object is not getting double wrapped for some reason, but I have no idea why. Another reason might be that I made a mistake somewhere in the debugging, which I don't think is the case, but an option none the less.

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #11)
> Does that give you the information necessary for me to shim the
> nsIWebNavigation implementation in RemoteWebNavigation?

I will look into this tomorrow, and will find a what's going on. Hopefully I can fix this on platform side once I figured out the reason for this behavior and then your shim will just work as it is. Removing the needinfo flag from Bill for now.

Comment 13

[Gabor Krizsanits (INACTIVE)]

I'm still stuck here. First I thought maybe RemoteWebNavigation does not get "double reflected" for some reason. So I pulled the whole thing through XPCOM factory and tried the same thing. It didn't help. I also tried with other JS implemented XPCOM objects (instantiated with Components.classes) to be on the safe side. But I experience the same behavior, namely that the clasp in AddonWrapper InterposePtoperty (it's the clasp of the unwrapped target object) seems to be a plain JS object:

(lldb) print *clasp
(const js::Class) $8 = {
  name = 0x0000000109b6f59d "Object"
  flags = 67108864
  addProperty = 0x0000000000000000
  delProperty = 0x0000000000000000
  getProperty = 0x0000000000000000
  setProperty = 0x0000000000000000
  enumerate = 0x0000000000000000
  resolve = 0x0000000000000000
  mayResolve = 0x0000000000000000
  finalize = 0x0000000000000000
  call = 0x0000000000000000
  hasInstance = 0x0000000000000000
  construct = 0x0000000000000000
  trace = 0x0000000000000000
  spec = {
    createConstructor_ = 0x0000000107ff3750 (XUL`CreateObjectConstructor(JSContext*, JSProtoKey) at Object.cpp:1033)
    createPrototype_ = 0x0000000107ff3860 (XUL`CreateObjectPrototype(JSContext*, JSProtoKey) at Object.cpp:1045)
    constructorFunctions_ = 0x000000010bb93390
    constructorProperties_ = 0x0000000000000000
    prototypeFunctions_ = 0x000000010bb936b0
    prototypeProperties_ = 0x000000010bb938e0
    finishInit_ = 0x0000000107ff3ad0 (XUL`FinishObjectClassInit(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>) at Object.cpp:1078)
    flags = 0
  }
  ext = {
    isWrappedNative = false
    weakmapKeyDelegateOp = 0x0000000000000000
    objectMovedOp = 0x0000000000000000
  }
  ops = {
    lookupProperty = 0x0000000000000000
    defineProperty = 0x0000000000000000
    hasProperty = 0x0000000000000000
    getProperty = 0x0000000000000000
    setProperty = 0x0000000000000000
    getOwnPropertyDescriptor = 0x0000000000000000
    deleteProperty = 0x0000000000000000
    watch = 0x0000000000000000
    unwatch = 0x0000000000000000
    getElements = 0x0000000000000000
    enumerate = 0x0000000000000000
    funToString = 0x0000000000000000
  }
}

I would expect it to be a WN class. And I have no clue why it isn't, maybe AddonWrappers do something tricky? (next thing I will look into). But it seems like AddonWrapper is not getting the double reflected version of the object. And this is concerning...

(In reply to Bobby Holley (busy) from comment #10)
> QI to nsIXPConnectWrappedJS. See UnwrapNativeCPOW for an example.

I was just wondering if I do the right thing, and to detect if a JSObject handle, is a double reflected object I should just check it's clasp. But it seems like your expectations and mines are the same and something else happening.

Setting needInfo flag again since there is something very fishy going on here.

Comment 14

[Gabor Krizsanits (INACTIVE)]

Mike, sorry that this takes so long, I was not expecting complications like this... I will be on PTO tomorrow so I'm afraid this will take a bit longer :(

Comment 15

[Mike Conley (:mconley) (:⚙️)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #14)
> Mike, sorry that this takes so long, I was not expecting complications like
> this... I will be on PTO tomorrow so I'm afraid this will take a bit longer
> :(

That's alright - I can try to work with billm or bholley today to try to figure this out today. If not, we'll figure this out when you're back.

Comment 16

[Gabor Krizsanits (INACTIVE)]

Attachment: xpcshell test for double reflector

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #15)
> That's alright - I can try to work with billm or bholley today to try to
> figure this out today. If not, we'll figure this out when you're back.

Ok, I'll try to summarize the issue.

- We don't interpose on vanilla JS objects
- RemoteWebNavigation seems like a vanilla JS object (no double reflector)
- to apply double reflector on it I think we should use Cc to instantiate it
- by default WNs are not too flexible so usually wrappedJSObject is used on them and interposition on those does not seem to work (I attached a patch that modifies an interposition test to illustrate the issue if someone has the time to debug it)

question: how can we make interpose work on these kind of components?

Comment 17

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #16)
> - We don't interpose on vanilla JS objects
> - RemoteWebNavigation seems like a vanilla JS object (no double reflector)
> - to apply double reflector on it I think we should use Cc to instantiate it

This all sounds correct to me.

> - by default WNs are not too flexible so usually wrappedJSObject is used on
> them and interposition on those does not seem to work (I attached a patch
> that modifies an interposition test to illustrate the issue if someone has
> the time to debug it)

When you say "is used on them", do you mean just that "the component permits access to the JS implementation behind the XPCOM interface by setting wrappedJSObject = this", or that "the component does the above, _and_ the consumer pulls out the JS implementation by accessing wrappedJSObject before passing the thing to InterposeProperty"?

If it's the latter, the consumer needs to not do that in order for this to work right, because once we're dealing with the underlying JSObject, the machinery has no way of knowing that it was previously being used to represent an XPCOM interface. If it's the former, that would imply that some code around AddonWrapper is automatically extracting the JS-implementation if available, which sounds like a bug.

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #13)
> I also tried with
> other JS implemented XPCOM objects (instantiated with Components.classes) to
> be on the safe side. But I experience the same behavior, namely that the
> clasp in AddonWrapper InterposePtoperty (it's the clasp of the unwrapped
> target object) seems to be a plain JS object:

If I understand your experiments right, sounds like the former. If so, we need to figure out who is extracting the JSImplementation before interposition and stop them from doing that.

Comment 18

[Mike Conley (:mconley) (:⚙️)]

One of the things that bholley has pointed out is that we're instantiating a RemoteWebNavigation by new'ing the implementation directly from the RemoteWebNavigation JSM, as opposed to going through XPCOM and createInstance-ing it. By doing this, we seem to side-step our interposition layer.

He suspects that if we register RemoteWebNavigation as a traditional XPCOM component and use createInstance instead, we might get the interposition stuff for free. I'm going to whip something up and see if that will work. Wish me luck!

Comment 19

[Bobby Holley (:bholley)]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #18)
> One of the things that bholley has pointed out is that we're instantiating a
> RemoteWebNavigation by new'ing the implementation directly from the
> RemoteWebNavigation JSM, as opposed to going through XPCOM and
> createInstance-ing it. By doing this, we seem to side-step our interposition
> layer.

Specifically, you're not creating a double-wrapped XPCOM component, and so the thing appears as a vanilla JS object to the interposition code.

Comment 20

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8705153 [details]
MozReview Request: Bug 1233803 - Register RemoteWebNavigation as a standard js-implemented XPCOM component. r?felipe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29885/diff/1-2/

Comment 21

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8703799 [details]
MozReview Request: Bug 1233803 - Outlaw usage of sessionHistory CPOW in browser code. r?felipe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29461/diff/2-3/

Comment 22

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8703800 [details]
MozReview Request: Bug 1233803 - Add RemoteWebNavigation sessionHistory shim for addons. r?krizsa

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29463/diff/2-3/

Comment 23

[Gabor Krizsanits (INACTIVE)]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #18)
> One of the things that bholley has pointed out is that we're instantiating a
> RemoteWebNavigation by new'ing the implementation directly from the
> RemoteWebNavigation JSM, as opposed to going through XPCOM and
> createInstance-ing it. By doing this, we seem to side-step our interposition
> layer.
> 
> He suspects that if we register RemoteWebNavigation as a traditional XPCOM
> component and use createInstance instead, we might get the interposition
> stuff for free. I'm going to whip something up and see if that will work.
> Wish me luck!

I've noticed that and pointed it out earlier too, in fact I had a patch that
does this but did not work out somehow, I wonder why. Anyway, the important
thing that we seem to do this patter all over the place, so I wanted to ask
what was the purpose for RemoteWebNavigation to all the dance of QI + wrappedJSObject
initially?

Comment 24

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8703800 [details]
MozReview Request: Bug 1233803 - Add RemoteWebNavigation sessionHistory shim for addons. r?krizsa

https://reviewboard.mozilla.org/r/29463/#review27489

r=me if any of my suggestions work for the target.wrappedJSObject line. If I misinterpreted that line or you want to do it differently, please let me know.

::: toolkit/components/addoncompat/RemoteAddonsParent.jsm:964
(Diff revision 3)
> +  let impl = target.wrappedJSObject;
> +  if (!impl) {
> +    return target.sessionHistory;
> +  }

Is this the case when we're in non e10s mode? The funny (I mean horrible) thing about wrappedJSObject that is used for these double reflected objects but also for xray wrappers (for completly different things). So this code might just waives the xray wrapper of a native WebNavigation object.

Can we do instead either: a) use instanceof (might not be easy because of scopes) b) check classID c) check if e10s is on or off.

Comment 25

[Mike Conley (:mconley) (:⚙️)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #23)
> (In reply to Mike Conley (:mconley) - Needinfo me! from comment #18)
> > One of the things that bholley has pointed out is that we're instantiating a
> > RemoteWebNavigation by new'ing the implementation directly from the
> > RemoteWebNavigation JSM, as opposed to going through XPCOM and
> > createInstance-ing it. By doing this, we seem to side-step our interposition
> > layer.
> > 
> > He suspects that if we register RemoteWebNavigation as a traditional XPCOM
> > component and use createInstance instead, we might get the interposition
> > stuff for free. I'm going to whip something up and see if that will work.
> > Wish me luck!
> 
> I've noticed that and pointed it out earlier too,

Sorry for not catching that. :(

> in fact I had a patch that
> does this but did not work out somehow, I wonder why. Anyway, the important
> thing that we seem to do this patter all over the place, so I wanted to ask
> what was the purpose for RemoteWebNavigation to all the dance of QI +
> wrappedJSObject
> initially?

Looking back through the blame, it looks like the thing that eventually became RemoteWebNavigation.jsm was originally an Object in the remote-browser XBL binding that got split out into a JSM for cleanliness. I don't think there was an explicit reason to just implement the interface in JS without registering the component.

Comment 26

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8705153 [details]
MozReview Request: Bug 1233803 - Register RemoteWebNavigation as a standard js-implemented XPCOM component. r?felipe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29885/diff/2-3/

Comment 27

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8703799 [details]
MozReview Request: Bug 1233803 - Outlaw usage of sessionHistory CPOW in browser code. r?felipe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29461/diff/3-4/

Comment 28

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8703800 [details]
MozReview Request: Bug 1233803 - Add RemoteWebNavigation sessionHistory shim for addons. r?krizsa

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29463/diff/3-4/

Comment 29

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1233803 - Add sessionHistory shim for gBrowser and remote browsers. r?krizsa

Review commit: https://reviewboard.mozilla.org/r/30747/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/30747/

Comment 30

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/29463/#review27489

> Is this the case when we're in non e10s mode? The funny (I mean horrible) thing about wrappedJSObject that is used for these double reflected objects but also for xray wrappers (for completly different things). So this code might just waives the xray wrapper of a native WebNavigation object.
> 
> Can we do instead either: a) use instanceof (might not be easy because of scopes) b) check classID c) check if e10s is on or off.

I'll instanceof the target to nsIDocShell - if it resolves to true, I guess I know that we're looking at an in-process browser. Sound good?

Comment 31

[Gabor Krizsanits (INACTIVE)]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #30)
> I'll instanceof the target to nsIDocShell - if it resolves to true, I guess
> I know that we're looking at an in-process browser. Sound good?

Yes, that sounds perfect.

Comment 32

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8705153 [details]
MozReview Request: Bug 1233803 - Register RemoteWebNavigation as a standard js-implemented XPCOM component. r?felipe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29885/diff/3-4/

Comment 33

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8703799 [details]
MozReview Request: Bug 1233803 - Outlaw usage of sessionHistory CPOW in browser code. r?felipe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29461/diff/4-5/

Comment 34

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8703800 [details]
MozReview Request: Bug 1233803 - Add RemoteWebNavigation sessionHistory shim for addons. r?krizsa

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29463/diff/4-5/

Comment 35

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8707536 [details]
MozReview Request: Bug 1233803 - Add sessionHistory shim for gBrowser and remote browsers. r?krizsa

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30747/diff/1-2/

Comment 36

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8705153 [details]
MozReview Request: Bug 1233803 - Register RemoteWebNavigation as a standard js-implemented XPCOM component. r?felipe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29885/diff/4-5/

Comment 37

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8703799 [details]
MozReview Request: Bug 1233803 - Outlaw usage of sessionHistory CPOW in browser code. r?felipe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29461/diff/5-6/

Comment 38

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8703800 [details]
MozReview Request: Bug 1233803 - Add RemoteWebNavigation sessionHistory shim for addons. r?krizsa

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29463/diff/5-6/

Comment 39

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8707536 [details]
MozReview Request: Bug 1233803 - Add sessionHistory shim for gBrowser and remote browsers. r?krizsa

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30747/diff/2-3/

Comment 40

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8707536 [details]
MozReview Request: Bug 1233803 - Add sessionHistory shim for gBrowser and remote browsers. r?krizsa

https://reviewboard.mozilla.org/r/30747/#review27655

Comment 41

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8705153 [details]
MozReview Request: Bug 1233803 - Register RemoteWebNavigation as a standard js-implemented XPCOM component. r?felipe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29885/diff/5-6/

Comment 42

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8703799 [details]
MozReview Request: Bug 1233803 - Outlaw usage of sessionHistory CPOW in browser code. r?felipe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29461/diff/6-7/

Comment 43

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8703800 [details]
MozReview Request: Bug 1233803 - Add RemoteWebNavigation sessionHistory shim for addons. r?krizsa

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29463/diff/6-7/

Comment 44

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8707536 [details]
MozReview Request: Bug 1233803 - Add sessionHistory shim for gBrowser and remote browsers. r?krizsa

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30747/diff/3-4/

Comment 45

[Mike Conley (:mconley) (:⚙️)]

Hey gabor, I had to update that last shims patch to fix a permaorange, because I didn't account for the non-e10s case. Can I assume your r+ still stands?

Interdiff: https://reviewboard.mozilla.org/r/30747/diff/3-4/

Comment 46

[Gabor Krizsanits (INACTIVE)]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #45)
> Hey gabor, I had to update that last shims patch to fix a permaorange,
> because I didn't account for the non-e10s case. Can I assume your r+ still
> stands?
> 
> Interdiff: https://reviewboard.mozilla.org/r/30747/diff/3-4/

LGTM

Comment 47

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8703799 [details]
MozReview Request: Bug 1233803 - Outlaw usage of sessionHistory CPOW in browser code. r?felipe

https://reviewboard.mozilla.org/r/29461/#review28017

Comment 48

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8705153 [details]
MozReview Request: Bug 1233803 - Register RemoteWebNavigation as a standard js-implemented XPCOM component. r?felipe

https://reviewboard.mozilla.org/r/29885/#review28019

::: toolkit/content/widgets/browser.xml:1181
(Diff revision 6)
>                "_remoteWebNavigation",
> +              "_remoteWebNavigationImpl",

do you need to remove _remoteWebNavigation from here?

Comment 49

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/29885/#review28019

> do you need to remove _remoteWebNavigation from here?

Nope - we still assign that in the constructor for the remote-browser binding. Consumers of the nsIWebNavigation interface will use that, but the \_remoteWebNavigationImpl is useful for when we need to fiddle with the RemoteWebNavigation instance behind the scenes.

Comment 50

[Mike Conley (:mconley) (:⚙️)]

https://hg.mozilla.org/integration/fx-team/rev/3ba697fefce2e891796b3f3f7bb7142bc952a7e7
Bug 1233803 - Register RemoteWebNavigation as a standard js-implemented XPCOM component. r=felipe

https://hg.mozilla.org/integration/fx-team/rev/df24394939b1fd83240ee24f070fb3e99bffd285
Bug 1233803 - Outlaw usage of sessionHistory CPOW in browser code. r=felipe

https://hg.mozilla.org/integration/fx-team/rev/7172bb64336d335f7ff3a2a5ef9d1f4699d45a59
Bug 1233803 - Add RemoteWebNavigation sessionHistory shim for addons. r=krizsa

https://hg.mozilla.org/integration/fx-team/rev/5d48bcc8521af3239f1c197a3cb90897afcafb12
Bug 1233803 - Add sessionHistory shim for gBrowser and remote browsers. r=krizsa

Comment 51

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3ba697fefce2
https://hg.mozilla.org/mozilla-central/rev/df24394939b1
https://hg.mozilla.org/mozilla-central/rev/7172bb64336d
https://hg.mozilla.org/mozilla-central/rev/5d48bcc8521a

Comment 52

[Brad Lassey [:blassey] (use needinfo?)]

Mike lets get this uplifted to 45

Comment 53

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8703799 [details]
MozReview Request: Bug 1233803 - Outlaw usage of sessionHistory CPOW in browser code. r?felipe

Approval Request Comment
[Feature/regressing bug #]:

None.

[User impact if declined]:

Slightly more IPC traffic than necessary.

[Describe test coverage new/current, TreeHerder]:

There's plenty of test coverage for the sessionHistory property in the tree, and that'll make use of our shimming layer, so we know it works.

[Risks and why]: 

Very little.

[String/UUID change made/needed]:

None.

Comment 54

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8703800 [details]
MozReview Request: Bug 1233803 - Add RemoteWebNavigation sessionHistory shim for addons. r?krizsa

Approval Request Comment
[Feature/regressing bug #]:

None.

[User impact if declined]:

Slightly more IPC traffic than necessary.

[Describe test coverage new/current, TreeHerder]:

There's plenty of test coverage for the sessionHistory property in the tree, and that'll make use of our shimming layer, so we know it works.

[Risks and why]: 

Very little.

[String/UUID change made/needed]:

None.

Comment 55

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8705153 [details]
MozReview Request: Bug 1233803 - Register RemoteWebNavigation as a standard js-implemented XPCOM component. r?felipe

Approval Request Comment
[Feature/regressing bug #]:

None.

[User impact if declined]:

Slightly more IPC traffic than necessary.

[Describe test coverage new/current, TreeHerder]:

There's plenty of test coverage for the sessionHistory property in the tree, and that'll make use of our shimming layer, so we know it works.

[Risks and why]: 

Very little.

[String/UUID change made/needed]:

None.

Comment 56

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8707536 [details]
MozReview Request: Bug 1233803 - Add sessionHistory shim for gBrowser and remote browsers. r?krizsa

Approval Request Comment
[Feature/regressing bug #]:

None.

[User impact if declined]:

Slightly more IPC traffic than necessary.

[Describe test coverage new/current, TreeHerder]:

There's plenty of test coverage for the sessionHistory property in the tree, and that'll make use of our shimming layer, so we know it works.

[Risks and why]: 

Very little.

[String/UUID change made/needed]:

None.

Comment 57

[Ryan VanderMeulen [:RyanVM]]

This needs some Aurora rebasing love.

Comment 58

[Mike Conley (:mconley) (:⚙️)]

You betcha. On it.

Comment 59

[Mike Conley (:mconley) (:⚙️)]

Attachment: aurora-patch.diff

Comment 60

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8710148 [details] [diff] [review]
aurora-patch.diff

Works great, thanks :)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2303ef84d6b3&group_state=expanded

Comment 61

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8707536 [details]
MozReview Request: Bug 1233803 - Add sessionHistory shim for gBrowser and remote browsers. r?krizsa

needed for the experiment.

Comment 62

[Mike Conley (:mconley) (:⚙️)]

Thanks!

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/c3b294d17ea4
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/67d28b544a8b
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/5f103cbb7ad1
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/8d81327764e1

Comment 63

[Mayur Patil]

[bugday-20160323]

Status: RESOLVED,FIXED -> UNVERIFIED

Comments:
STR: Not clear.
Developer specific testing

Component: 
Name			Firefox
Version			46.0b9
Build ID		20160322075646
Update Channel	beta
User Agent		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS				Windows 7 SP1 x86_64

Expected Results: 
Developer specific testing

Actual Results: 
As expected

Comment 64

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bugnotes

http://people.mozilla.org/~mconley2/bugnotes/bug-1233803.html