Currently docshell uses the term 'owner' but the loadInfo uses the term TriggeringPrincipal. Would it make sense to update |nsISupports owner;| to become |nsIPrincipal triggeringPrincipal;| [1]? I crafted a first patch and it seems owner is always an nsIPrincipal so I guess there is no need that it remains an nsISupports, or am I forgetting about something? [1] https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsIDocShell.idl#164

Boris, any objections on replacing |nsiSupports owner;| with |nsIPrincipal triggeringPrincipal| for docshell loads?

No, makes total sense to me.

Boris, replacing the owner within nsIDocShell.idl goes hand in hand with also updating: * nsIDocShellLoadInfo.idl * nsIWebNavigation.idl * nsISHEntry.idl As a follow up I filed Bug 1286838 within which we can then deprecate 'owner' on nsIChannel once we have converted docshell loads to rely on asyncOpen2(). https://treeherder.mozilla.org/#/jobs?repo=try&revision=5673030830f7

Comment on attachment 8770974 [details] [diff] [review] bug_1286472_replace_owner_with_triggeringprincipal.patch >+++ b/docshell/base/nsDocShell.cpp >+ // (4) we pass a null principal into the channel, and a principal will be This is easy to confuse with a NullPrincipal. Maybe "we don't pass a principal into the channel"? This occurs in two places. >+ // NOTE: This all only works because the only thing the triggeringPrincipal >+ // is used for in InternalLoad is data:, javascript:, and about:blank This comment is no longer true, right? Those are the only things that the triggeringPrincipal is _inherited_ by, but we _use_ it for everything, by putting it in the loadinfo. >+ nsCOMPtr<nsIPrincipal> ownerPrincipal = triggeringPrincipal; You don't need the ownerPrincipal thing anymore. Just use triggeringPrincipal, since you no longer need to QI. +++ b/docshell/base/nsIWebNavigation.idl >- const unsigned long LOAD_FLAGS_DISALLOW_INHERIT_OWNER = 0x40000; This will break addons. See https://dxr.mozilla.org/addons/search?q=DISALLOW_INHERIT_OWNER You probably want to just have both constants here, with the same value and a comment. r=me with that. Thank you for doing this!

(In reply to Boris Zbarsky [:bz] from comment #4) > >+ nsCOMPtr<nsIPrincipal> ownerPrincipal = triggeringPrincipal; > > You don't need the ownerPrincipal thing anymore. Just use > triggeringPrincipal, since you no longer need to QI. That part within nsDocShell::LoadURI() seems fragile, e.g. further down is this part: > if (aLoadFlags & LOAD_FLAGS_DISALLOW_INHERIT_PRINCIPAL) { > inheritPrincipal = false; > triggeringPrincipal = ownerPrincipal ? > nsNullPrincipal::CreateWithInheritedAttributes(ownerPrincipal) : > nsNullPrincipal::CreateWithInheritedAttributes(this); > } I am too scared to refactor those bits because it seems not very robust and I think we should rather keep the tmp variable 'ownerPrincipal' within this 'renaming' patch and really don't change any logic. If you really think this part is worth updating, then I am happy to file a follow up.

Boris, when running > browser/components/sessionstore/test/browser_911547.js I realized we don't serialize correctly because code within SessionHistory was relying on entry.owner which has now become .triggeringPrincipal. Sorry, I missed that the first time around!

> e.g. further down is this part Ah, indeed. Can you please rename ownerPrincipal to originalTriggeringPrincipal or principalToInheritAttributesFrom, then? That's what we're really using it for, after all.

Comment on attachment 8771960 [details] [diff] [review] bug_1286472_serialize_triggeringprincipal_not_owner.patch >+ if (entry.triggeringPrincipal_b64) { No, this is wrong. The reason it's wrong is consider what happens during the update from a version of Firefox without this change to a version with. The serialized data will have entry.owner_b64 but here we'll look for entry.triggeringPrincipal_b64. We need to look for both, probably preferring the latter. There's a similar issue in the Android code. Also, if someone _downgrades_ Firefox this could lose parts of the session, afaict: the serialization will have entry.triggeringPrincipal_b64 but the downgraded deserialization code will look for entry.owner_b64. It might sense to leave the property named "owner_b64" in the serialization... Please get review from actual owners of this code on this stuff.

Comment on attachment 8771960 [details] [diff] [review] bug_1286472_serialize_triggeringprincipal_not_owner.patch Not my area of expertise. @snorp: Can you review this or find a reviewer? :)

Comment on attachment 8771960 [details] [diff] [review] bug_1286472_serialize_triggeringprincipal_not_owner.patch Review of attachment 8771960 [details] [diff] [review]: ----------------------------------------------------------------- I don't really know this code, but if it's mostly s/owner/triggeringPrincipal/ I guess it seems fine?

Except see comment 9.

Redirecting to Mike who is responsible for sessionstore nowadays. I unfortunately course can't do this properly because he blocked review/needinfo requests. Mike, please check comment #9, Boris makes some really good points especially about what happens when loading an old session after upgrading. I personally wouldn't be too worried about the downgrade scenario but that's your call to make now :)

Comment on attachment 8771960 [details] [diff] [review] bug_1286472_serialize_triggeringprincipal_not_owner.patch Review of attachment 8771960 [details] [diff] [review]: ----------------------------------------------------------------- Just sent Mike an email with a link to the previous comment.

Comment on attachment 8771960 [details] [diff] [review] bug_1286472_serialize_triggeringprincipal_not_owner.patch Review of attachment 8771960 [details] [diff] [review]: ----------------------------------------------------------------- I can't r+ this just yet, because I'd like to hear your opinion on my comment regarding supporting owner_b64 for older sessionstore.js reads. Other than that this looks good. ::: browser/components/sessionstore/SessionHistory.jsm @@ +378,5 @@ > childDocIdents = matchingEntry.childDocIdents; > } > } > > + if (entry.triggeringPrincipal_b64) { Well, I think there's a possibility that we'll get sessionstore.js entries with `entry.owner_b64` in there still. So we'll need to be backward compatible for a while by doing something like: ```js // This field was called 'owner_b64', so read that field if it's still there. if (entry.owner_b64) { entry.triggeringPricipal_b64 = entry.owner_b64; delete entry.owner_b64; } if (entry.triggeringPrincipal_b64) { ... ``` ::: mobile/android/components/SessionStore.js @@ +1251,5 @@ > } > > + if (aEntry.triggeringPrincipal_b64) { > + let triggeringPrincipalInput = Cc["@mozilla.org/io/string-input-stream;1"]. > + createInstance(Ci.nsIStringInputStream); nit: appreciate the formatting, but can you write it like: ``js let triggeringPrincipalInput = Cc["@mozilla.org/io/string-input-stream;1"] .createInstance(Ci.nsIStringInputStream); ``` ? @@ +1255,5 @@ > + createInstance(Ci.nsIStringInputStream); > + let binaryData = atob(aEntry.triggeringPrincipal_b64); > + triggeringPrincipalInput.setData(binaryData, binaryData.length); > + let binaryStream = Cc["@mozilla.org/binaryinputstream;1"]. > + createInstance(Ci.nsIObjectInputStream); nit: same here. @@ +1263,1 @@ > } catch (ex) { dump(ex); } While you're here, can you change the 'dump' here to `Cu.reportError("SessionStore: " + ex);`?

Just renamed ownerPrincipal to principalToInheritAttributesFrom. Carrying over r+ from bz.

Mike, thanks for the feedback. I incorporated all you suggestions and nits.

Comment on attachment 8775151 [details] [diff] [review] bug_1286472_serialize_triggeringprincipal_not_owner.patch Review of attachment 8775151 [details] [diff] [review]: ----------------------------------------------------------------- I like it, thanks! ::: browser/components/sessionstore/SessionHistory.jsm @@ +378,5 @@ > childDocIdents = matchingEntry.childDocIdents; > } > } > > + // The field entry.owner_b64 got renamed to entry.triggeringPrincipal within nit: entry.triggeringPrincipal_b64 and s/within/in/

Boris, I think you and Mike got it all covered and we are still backward compatible by providing > if (entry.owner_b64) { > entry.triggeringPricipal_b64 = entry.owner_b64; > delete entry.owner_b64; > } Do we need to wait for snorp's feedback, or can we land those changesets?

>+ // The field entry.owner_b64 got renamed to entry.triggeringPrincipal within No, it got renamed to entry.triggeringPricipal_b64. If Mike is ok with this, and in particular if he feels like he can make this call for Fennec, we can probably just land this.

(In reply to Boris Zbarsky [:bz] from comment #20) > >+ // The field entry.owner_b64 got renamed to entry.triggeringPrincipal within > > No, it got renamed to entry.triggeringPricipal_b64. Oh, indeed. Thanks for catching this. I will update that in the final patch.

(In reply to Boris Zbarsky [:bz] from comment #20) > No, it got renamed to entry.triggeringPricipal_b64. The review comments I made before for SessionHistory.jsm also apply here, so please apply them, Christoph. Thanks! > If Mike is ok with this, and in particular if he feels like he can make this > call for Fennec, we can probably just land this. Hmmm, I don't know if my newly acquired module ownership extends to Fennec as well, but I think it's fine in this case.

Pushed by mozilla@christophkerschbaumer.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/edb10ed57ed4 Replace owner with triggeringPrincipal within docshell. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/5edd4f667589 Replace serializing nsISHEntry.owner with nsISHEntry.triggeringPrincipal. r=bz,mikedeboer

https://hg.mozilla.org/mozilla-central/rev/edb10ed57ed4 https://hg.mozilla.org/mozilla-central/rev/5edd4f667589