Important to reference is: https://bugzilla.mozilla.org/show_bug.cgi?id=1105556#c23 In summary, it mentions that ctrl+r or also ctrl+shift+R the sequence of calls is: nsSHistory::Reload nsSHistory::LoadEntry nsSHistory::InitiateLoad nsDocShell::LoadURI nsDocShell::LoadHistoryEntry nsDocShell::InternalLoad It seems that InitateLoad does not pass the triggeringPrincipal or also not the principalToInherit from the session history entry to the docshell loadinfo: https://dxr.mozilla.org/mozilla-central/source/docshell/shistory/nsSHistory.cpp#1753

Hi Chris, Any update on this bug?

As discussed in our meeting, we should deny the load if the entry does not hold a valid triggeringPrincipal. Let's see how TRY does: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad11c975c26226b061cf7915c5416e3d3437d351

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

Boris, it seems we can enforce that history entries have a valid triggeringPrincipal by only updating 2 things. I am not entirely sure though if I am using the right triggeringPrincipal in those two cases: a) nsDocShell::AddState() is the only place that calls AddToSessionHistory() without providing a valid triggeringPrincipal. Looking through the code of AddState() I think using document->NodePrincipal() as the triggeringPrincipal as well as principalToInherit is the right choice. b) nsDocShell::CreateContentViewer potentially calls onNewURI() where failedChannel is null. Since the argument triggeringPrincipal is always null, we end up with a null triggeringPrincipal on the history entry. E.g. within test browser_moz_action_link.js we fall back to using 'about:blank' since "failedURI" is null. In that case we would end up with a nullptr for failedChannel as well as a an explicit nullptr for triggeringPrincipal. If desired, we could also move the triggeringPrincipal creation to within: if (!failedURI) { // We need a URI object to store a session history entry, so make up a URI NS_NewURI(getter_AddRefs(failedURI), "about:blank"); triggeringPrincial = nsContentUtils::GetSystemPrincipal(); } That approach has the downside that there is potentially a codepath where we still end up with a null failedChannel and a null triggeringPrincipal. Upside of that approach is, that we would know explicitly, but potentially history loads get denied. Since we are forcing history entries to have a valid triggeringPrincipal, we have to update some tests to provide an explicit triggeringprincipal for those tests that create a history entry.

Using document->NodePrincipal() for the triggering principal in AddState() makes sense. I'm not sure it makes sense to use it for the principalToInherit. It may make more sense to use null for principalToInherit there. Why would you ever want the result of the AddState() call to inherit the current document's principal on reload? > nsDocShell::CreateContentViewer potentially calls onNewURI() where failedChannel is null. This can basically only happen when loading an error page and either the error is "could not create an nsIURI from this URI" or "tried to navigate during printing or print preview". In either case, we're doing an error page load, and should probably use system principal for the triggering principal. I don't see how this patch forces session history entries to have a valid triggering principal, though. Random chrome JS (e.g. in extensions) can just create them without a triggering principal, no? Asserts won't catch that.

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #7) > I don't see how this patch forces session history entries to have a valid > triggering principal, though. Random chrome JS (e.g. in extensions) can > just create them without a triggering principal, no? Asserts won't catch > that. Agreed, it's hard to enforce that all entries have a valid triggeringPrincipal at creation time of an entry. At least for code within our codebase we have an assertion which allows to pinpoint the actual problem right away instead of waiting for the entry to be used within LoadHistoryEntry() which would then deny the load if the entry does not have a valid triggeringPrincipal. Agreed?

> At least for code within our codebase we have an assertion which allows to pinpoint the actual problem right away We do? Where? Anyway, we can't rely on assertions here, afaict. We need to actually enforce things somewhere, not just assert them.

Comment on attachment 8809838 [details] [diff] [review] bug_1307736_assert_history_loads_pass_valid_triggeringPrincipal.patch (In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #9) > > At least for code within our codebase we have an assertion which allows to pinpoint the actual problem right away > > We do? Where? > > Anyway, we can't rely on assertions here, afaict. We need to actually > enforce things somewhere, not just assert them. Wow, last update on this bug was several weeks ago :-( Let me try to summarize my question real quick: I agree, we can't enforce that random JS code creates history entries, but we can enforce that a history entry has a valid triggeringPrincipal within LoadHistoryEntry and otherwise abort the load. Within LoadHistoryEntry we don't only assert but we also return an error. Isn't that what we want ultimately?

[Tracking Requested - why for this release]: Because https://bugzilla.mozilla.org/show_bug.cgi?id=1182569 landed and I'm worried about missing triggeringPrincipals that fall back to system, please land this in 53.

Sorry for the lag here.... > we can enforce that a history entry has a valid triggeringPrincipal within LoadHistoryEntry and otherwise abort the load. Yes, agreed. I see now that we are in fact doing that in LoadHistoryEntry. OK, then this seems pretty reasonable.

Tracking 53+ per Comment 11.

Comment on attachment 8826197 [details] [diff] [review] bug_1307736_assert_history_loads_pass_valid_triggeringPrincipal.patch >+ // a triggeringPrincipal for the history entry, e.g. for >+ // 'about:blank' loads. I don't see how about:blank could ever end up here, much less without a failedChannel. What is this comment really trying to say? >+ if(!triggeringPrincipal) { Space after "if", please. > nsSHEntry::SetTriggeringPrincipal(nsIPrincipal* aTriggeringPrincipal) Could have this return failure on null too, in addition to asserting. r=me with that comment fixed to make sense...

Comment on attachment 8826198 [details] [diff] [review] bug_1307736_test_updates.patch r=me, but might be good to have the sessionstore folks review this too. In any case, I assume you'll fold it with the other patch when landing, to avoid a bad intermediate state, right?

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #17) > I don't see how about:blank could ever end up here, much less without a > failedChannel. What is this comment really trying to say? I tried to explain within comment 5 b). Anyway, I moved the creation of the principal exactly after the line where we fall back to about:blank to be more explicit in what case we need the principal fallback. Carrying over r+ from bz.

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #18) > r=me, but might be good to have the sessionstore folks review this too. Mike, can you be that person to have a look at those tests to make sure everything is fine? > In any case, I assume you'll fold it with the other patch when landing, to > avoid a bad intermediate state, right? I will land the two patches together, yes.

Comment on attachment 8826509 [details] [diff] [review] bug_1307736_test_updates.patch Review of attachment 8826509 [details] [diff] [review]: ----------------------------------------------------------------- I'd like to have one more quick look at the final patch. Thanks! Can you show us a green try build? ::: browser/components/preferences/in-content/tests/browser_bug1184989_prevent_scrolling_when_preferences_flipped.js @@ +1,3 @@ > const ss = Cc["@mozilla.org/browser/sessionstore;1"].getService(Ci.nsISessionStore); > > +var {Utils} = Cu.import("resource://gre/modules/sessionstore/Utils.jsm", {}); nit: `const` is fine to use here too. @@ +1,5 @@ > const ss = Cc["@mozilla.org/browser/sessionstore;1"].getService(Ci.nsISessionStore); > > +var {Utils} = Cu.import("resource://gre/modules/sessionstore/Utils.jsm", {}); > +const SYSTEMPRINCIPAL = Services.scriptSecurityManager.getSystemPrincipal(); > +let triggeringPrincipal = Utils.serializePrincipal(SYSTEMPRINCIPAL); Please use `var` or `const` in the root scope. Some comment applies to the other test files ;) ::: browser/components/sessionstore/test/browser_394759_purge.js @@ +3,5 @@ > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > Components.utils.import("resource://gre/modules/ForgetAboutSite.jsm"); > > +var {Utils} = Cu.import("resource://gre/modules/sessionstore/Utils.jsm", {}); Can you put this import and the two constants below at the top of the head.js file in browser/components/sessionstore/test/head.js ?

Comment on attachment 8826508 [details] [diff] [review] bug_1307736_assert_history_loads_pass_valid_triggeringPrincipal.patch OK, this is somewhat clearer. There can certainly be cases in which we have a failedURI but not a failedChannel: any time the NS_NewChannel failed after NS_NewURI succeeded. I think if we want to set triggeringPrincipal to system exactly when failedChannel is null (which would make sense), then we should do exactly that. So move the new bit into the "else" of the "if (failedChannel) {" block.

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #22) > I think if we want to set triggeringPrincipal to system exactly when > failedChannel is null (which would make sense), then we should do exactly > that. So move the new bit into the "else" of the "if (failedChannel) {" > block. Agreed - carrying over r+ from bz.

(In reply to Mike de Boer [:mikedeboer] from comment #21) > Can you show us a green try build? https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ad241df12b6df7398f1fe1add59ca3157ece304 > > +var {Utils} = Cu.import("resource://gre/modules/sessionstore/Utils.jsm", {}); > nit: `const` is fine to use here too. Updated to use const everywhere in the root scope. > ::: browser/components/sessionstore/test/browser_394759_purge.js > > +var {Utils} = Cu.import("resource://gre/modules/sessionstore/Utils.jsm", {}); > > Can you put this import and the two constants below at the top of the > head.js file in browser/components/sessionstore/test/head.js ? Most certainly - thanks for the speedy review.

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #24) > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=5ad241df12b6df7398f1fe1add59ca3157ece304 Well, that's not entirely green I say ;-)

Comment on attachment 8827070 [details] [diff] [review] bug_1307736_test_updates.patch Review of attachment 8827070 [details] [diff] [review]: ----------------------------------------------------------------- When you moved the constants to head.js, you forgot to remove them from the individual test files. So if you do that, it's likely that you mochitest-bc suites will turn green again. It usually pays off to run the tests you changed locally for a quick verification before you push them all to Try. r=me with that change. I don't know why the Marionette tests are timing out - its signature is different from an intermittent.

(In reply to Mike de Boer [:mikedeboer] from comment #26) > suites will turn green again. > It usually pays off to run the tests you changed locally for a quick > verification before you push them all to Try. Totally agree, just verified browser_394759_purge.js that's where you pointed out I should move it to the head.js.

Pushed by mozilla@christophkerschbaumer.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/1a71a0d9aa90 Ensure History loads pass valid triggeringPrincipal. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/338dd2c343f8 Ensure tests performing a history load pass valid triggeringPrincipal. r=bz,mdeboer

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/28ace1abfb5c for https://treeherder.mozilla.org/logviewer.html#?job_id=69346504&repo=mozilla-inbound and https://treeherder.mozilla.org/logviewer.html#?job_id=69346532&repo=mozilla-inbound

Dunno how come I missed browser/components/sessionstore/test/browser_490040.js. Anyway, I updated this test in the same way we updated all the other tests. I imagine mdeboer is fine with that. Qfolded that change in this patch and carrying over r+ from mdeboer.

Localizing and fixing the other problem is harder, the problem is buried within test_refresh_firefox.py. We are creating a history entry of "about:welcomeback" [1] which does not have a triggeringPrincipal and hence we do not allow the load. Gijs, you wrote most of that test, any chance you could provide some pointers how I could debug that? I think we must create an nsISHEntry somewhere but only set the URI on it but *not* the triggeringPrincipal and hence we don't allow the load. Any ideas? [1] https://dxr.mozilla.org/mozilla-central/source/browser/components/migration/tests/marionette/test_refresh_firefox.py#262

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #31) > Localizing and fixing the other problem is harder, the problem is buried > within test_refresh_firefox.py. We are creating a history entry of > "about:welcomeback" [1] which does not have a triggeringPrincipal and hence > we do not allow the load. > > Gijs, you wrote most of that test, any chance you could provide some > pointers how I could debug that? I think we must create an nsISHEntry > somewhere but only set the URI on it but *not* the triggeringPrincipal and > hence we don't allow the load. Any ideas? > > [1] > https://dxr.mozilla.org/mozilla-central/source/browser/components/migration/ > tests/marionette/test_refresh_firefox.py#262 It's not going to be a test problem, as it's not the test that does the loading of that file. This is the bottom of the rabbithole: https://dxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/SessionMigration.jsm#61-63 . I don't know off-hand exactly where we then restore that state, but I suspect this would mean your test might also break loading other chrome URIs from session restore, which might be easier to reproduce and figure out. That is, 1. create new profile 2. set to restore tabs on startup 3. open e.g. about:preferences (the options/prefs) in a tab 4. restart firefox

Comment on attachment 8827069 [details] [diff] [review] bug_1307736_assert_history_loads_pass_valid_triggeringPrincipal.patch Even if this whole patch with assertions doesn't land in 53, can we just land this part: >@@ -11988,18 +11996,19 @@ nsDocShell::AddState(JS::Handle<JS::Valu > GetCurScrollPos(ScrollOrientation_Y, &cy); > mOSHE->SetScrollPosition(cx, cy); > > bool scrollRestorationIsManual = false; > mOSHE->GetScrollRestorationIsManual(&scrollRestorationIsManual); > > // Since we're not changing which page we have loaded, pass > // true for aCloneChildren. >- rv = AddToSessionHistory(newURI, nullptr, nullptr, nullptr, true, >- getter_AddRefs(newSHEntry)); >+ rv = AddToSessionHistory(newURI, nullptr, >+ document->NodePrincipal(), // triggeringPrincipal >+ nullptr, true, getter_AddRefs(newSHEntry)); > NS_ENSURE_SUCCESS(rv, rv); > > NS_ENSURE_TRUE(newSHEntry, NS_ERROR_FAILURE); > > // Session history entries created by pushState inherit scroll restoration > // mode from the current entry. > newSHEntry->SetScrollRestorationIsManual(scrollRestorationIsManual); >

(In reply to Tanvi Vyas - behind on bugmail [:tanvi] from comment #33) > Even if this whole patch with assertions doesn't land in 53, can we just > land this part: > > >- rv = AddToSessionHistory(newURI, nullptr, nullptr, nullptr, true, > >- getter_AddRefs(newSHEntry)); > >+ rv = AddToSessionHistory(newURI, nullptr, > >+ document->NodePrincipal(), // triggeringPrincipal > >+ nullptr, true, getter_AddRefs(newSHEntry)); I filed Bug 1332310 - Update TriggeringPrincipal when adding to session history within docshell.

As agreed with Tanvi and bz we are going to land a partial fix for this bug within Bug 1332310 [1]. The partial fix is why Tanvi requested tracking for 55 (see comment 33). Hence I will clear the tracking flags for this bug. We will land this bug within 54. [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1332310#c5

Clearing flags - see comment 35 for details.

Rebasing patches since Bug 1332310 landed. Carrying over r+ from bz.

After the backout of these patches I took a closer look and had to update a few more tests within browser/components/sessionstore/test, but they all follow the same schemata using the triggeringPrincipal defined within head.js. Carrying over r+ from bz and mdeboer.

(In reply to :Gijs from comment #32) > I don't know off-hand exactly where we then restore that state, but I > suspect this would mean your test might also break loading other chrome URIs > from session restore, which might be easier to reproduce and figure out. > That is, > > 1. create new profile > 2. set to restore tabs on startup > 3. open e.g. about:preferences (the options/prefs) in a tab > 4. restart firefox Hey Gijs, I followed those steps and found that we have to update some more code within sessionstore that creates and manipulates session history entries. I am not entirely sure if it's even OK to use the systemPrincipal for those loads. Anyway, at the moment restoring a session from Firefox works again (following your provided STR), but the marionette test test_refresh_firefox.py still fails (see details underneath). Any idea what I can do to debug, or how I can possibly find out where that error occurs? Thanks for your help! 1:04.48 TEST_END: MainThread FAIL, expected PASS Traceback (most recent call last): File "/home/ckerschb/moz/mc/testing/marionette/harness/marionette_harness/marionette_test/testcases.py", line 166, in run testMethod() File "/home/ckerschb/moz/mc/browser/components/migration/tests/marionette/test_refresh_firefox.py", line 418, in testReset self.checkProfile(hasMigrated=True) File "/home/ckerschb/moz/mc/browser/components/migration/tests/marionette/test_refresh_firefox.py", line 288, in checkProfile self.checkSession() File "/home/ckerschb/moz/mc/browser/components/migration/tests/marionette/test_refresh_firefox.py", line 278, in checkSession self.assertSequenceEqual(tabURIs, ["about:blank"] + self._expectedURLs) AssertionError: Sequences differ: [u'about:robots', u'about:mozi... != ['about:blank', 'about:robots'... First differing element 0: about:robots about:blank Second sequence contains 1 additional elements. First extra element 2: about:mozilla - [u'about:robots', u'about:mozilla'] ? - + ['about:blank', 'about:robots', 'about:mozilla']

I've seen that error locally myself but never on infra, and I haven't had time to figure out why it happens (in part because it's hard because marionette is REALLY GOOD at swallowing any kind of logging, it has no debugging infrastructure, and so figuring out the cause is basically like stabbing in the dark while drunk. I would suggest a trypush, and look more if it happens on infra, which seems unlikely.

Comment on attachment 8829581 [details] [diff] [review] bug_1307736_update_session_management.patch Review of attachment 8829581 [details] [diff] [review]: ----------------------------------------------------------------- This looks vaguely OK... might make sense to also doublecheck with mikedeboer when going for a 'real' r+. ::: browser/components/sessionstore/SessionMigration.jsm @@ +23,5 @@ > }); > > +const {Utils} = Cu.import("resource://gre/modules/sessionstore/Utils.jsm", {}); > +const SYSTEMPRINCIPAL = Services.scriptSecurityManager.getSystemPrincipal(); > +const serialized_triggeringPrincipal = Utils.serializePrincipal(SYSTEMPRINCIPAL); This looks unused? ::: browser/components/sessionstore/SessionStore.jsm @@ +188,5 @@ > "resource://gre/modules/ViewSourceBrowser.jsm"); > XPCOMUtils.defineLazyModuleGetter(this, "AsyncShutdown", > "resource://gre/modules/AsyncShutdown.jsm"); > > +const {Utils} = Cu.import("resource://gre/modules/sessionstore/Utils.jsm", {}); Just Cu.import without assigning or passing a scope object (here and elsewhere). @@ +190,5 @@ > "resource://gre/modules/AsyncShutdown.jsm"); > > +const {Utils} = Cu.import("resource://gre/modules/sessionstore/Utils.jsm", {}); > +const SYSTEMPRINCIPAL = Services.scriptSecurityManager.getSystemPrincipal(); > +const serialized_triggeringPrincipal = Utils.serializePrincipal(SYSTEMPRINCIPAL); XPCOMUtils.defineLazyGetter("SYSTEM_TRIGGERING_PRINCIPAL", () => { Cu.import("blah/utils.jsm"); return Utils.serializePrincipal(Services.scriptSecurityManager.getSystemPrincipal()); }); Here and elsewhere.

Thanks Gijs, it seems because of the latest changes we have to update a lot more tests within browser/components/sessionstore/test. Once I have a complete green try run I have to ask mdeboer to review those again. One step at a time, tests pass locally, let's see what try thinks: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0fe7f21fce02a71b646662bde9a6c35c2c42283d

No need for an additional ni? besides the feedback. Clearing the flag - thanks! \

Hey Mike, after the backout of the patch because of failing marionette tests [1] I took a closer look at what we are trying to accomplish here and I figured that we also have to modify some code within SessionHistory, SessionMigration and SessionStore to make sure we copy/pass around the correct triggeringPrincipal. Please see green try run here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1564de9d00a544f0f41ea755bf4cbad1a7a52db5 [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1307736#c31

Initially those tests were already r+, but i was wondering if you want to take another look since I basically updated all the tests within browser/components/sessionstore/test to pass a valid triggeringPrincipal. As you suggested I keep the initial creation of the triggeringPrincipal within browser/components/sessionstore/test/head.js and then use it in the all the tests. Since this change basically affects all of the tests now I am happy to update whatever concerns you may raise. Further I changed browser/components/sessionstore/test/browser_restore_cookies_noOriginAttributes.js to use JSON.stringify instead of passing a raw string. Lots of other tests do that and I think it's the only way to pass the needed 'triggeringPrincipal'. Thanks!

Comment on attachment 8830646 [details] [diff] [review] bug_1307736_update_session_management.patch Review of attachment 8830646 [details] [diff] [review]: ----------------------------------------------------------------- I really like the fact that you define SERIALIZED_SYSTEMPRINCIPAL as a lazy getter, but it would be just more awesome if you'd make it part of sessionstore/Utils.jsm: ```js XPCOMUtils.defineLazyGetter(this, "SERIALIZED_SYSTEMPRINCIPAL", function() { return Utils.serializePrincipal(Services.scriptSecurityManager.getSystemPrincipal()); }); this.Utils = Object.freeze({ get SERIALIZED_SYSTEMPRINCIPAL() { return SERIALIZED_SYSTEMPRINCIPAL; }, // ... }); ``` ::: browser/components/sessionstore/SessionMigration.jsm @@ +52,5 @@ > state.windows = aStateObj.windows.map(function(oldWin) { > var win = {extData: {}}; > win.tabs = oldWin.tabs.map(function(oldTab) { > var tab = {}; > // Keep only titles and urls for history entries nit: please update this comment. @@ +54,5 @@ > win.tabs = oldWin.tabs.map(function(oldTab) { > var tab = {}; > // Keep only titles and urls for history entries > tab.entries = oldTab.entries.map(function(entry) { > + return {url: entry.url, triggeringPrincipal_base64: entry.triggeringPrincipal_base64, title: entry.title}; nit: now you need to properly format this literal across multiple lines, I think. Or you do: ```js let {title, triggeringPrincipal_base64, url} = entry; return {title, triggeringPrincipal_base64, url}; ``` @@ +67,5 @@ > return win; > }); > let url = "about:welcomeback"; > let formdata = {id: {sessionData: state}, url}; > + return {windows: [{tabs: [{entries: [{url, triggeringPrincipal_base64: SERIALIZED_SYSTEMPRINCIPAL}], formdata}]}]}; nit: same, formatting needs to change to keep things readable. ::: browser/components/sessionstore/SessionStore.jsm @@ +631,5 @@ > if (this._needsRestorePage(state, this._recentCrashes)) { > // replace the crashed session with a restore-page-only session > let url = "about:sessionrestore"; > let formdata = {id: {sessionData: state}, url}; > + state = { windows: [{ tabs: [{ entries: [{url, triggeringPrincipal_base64: SERIALIZED_SYSTEMPRINCIPAL}], formdata }] }] }; nit: please update formatting.

Comment on attachment 8830647 [details] [diff] [review] bug_1307736_test_updates.patch Review of attachment 8830647 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/sessionstore/test/head.js @@ +1,5 @@ > /* This Source Code Form is subject to the terms of the Mozilla Public > * License, v. 2.0. If a copy of the MPL was not distributed with this > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > +const {Utils} = Cu.import("resource://gre/modules/sessionstore/Utils.jsm", {}); When you've updated the previous patch, you'll have to forward that change to _ALL_ these tests as well :( Terribly sorry about that, but there's no other way I can think of. You could make it a bit magic by doing `const triggeringPrincipal_base64 = Utils.SERIALIZED_SYSTEMPRINCIPAL;` and be able to do `{ url: "https://example.com", triggeringPrincipal_base64 }` in the tests to make the literals not grow as much. But perhaps that's too much magic? I don't know. I'll leave it up to you!

(In reply to Mike de Boer [:mikedeboer] from comment #48) > You could make it a bit magic by doing `const triggeringPrincipal_base64 = > Utils.SERIALIZED_SYSTEMPRINCIPAL;` and be able to do `{ url: > "https://example.com", triggeringPrincipal_base64 }` in the tests to make > the literals not grow as much. > But perhaps that's too much magic? I don't know. I'll leave it up to you! Thanks for the speedy reviews and those suggestions. I will get that 'magic' ready, but it might take some time, but i suppose it's fine if we land that early next week. Thanks again for your help here!

I had a few cycles and I think I updated all of your suggestions. If not, please let me know.

Comment on attachment 8830855 [details] [diff] [review] bug_1307736_update_session_management.patch Review of attachment 8830855 [details] [diff] [review]: ----------------------------------------------------------------- Very nice, thanks!

Comment on attachment 8830856 [details] [diff] [review] bug_1307736_test_updates.patch Review of attachment 8830856 [details] [diff] [review]: ----------------------------------------------------------------- Your changes made this _so_ much easier to review... many thanks! As a hunch, can you add Talos 'other' suite to your try push? There are sessionstore tests in there that I'd like to see working before checkin... Thanks, Christoph. ::: browser/components/sessionstore/test/browser_aboutSessionRestore.js @@ +44,1 @@ > is(url, CRASH_URL, "session was restored correctly"); This is array destructuring from the result of JSON.parse, so I think you can omit this change.

(In reply to Mike de Boer [:mikedeboer] from comment #53) > ::: browser/components/sessionstore/test/browser_aboutSessionRestore.js > > is(url, CRASH_URL, "session was restored correctly"); > This is array destructuring from the result of JSON.parse, so I think you > can omit this change. Indeed. So, since the changesets within this bug already got backed out once I think I am going all in and run *all* tests on try to make sure everything is fine before checking in: https://treeherder.mozilla.org/#/jobs?repo=try&revision=679a776d21c6b02671ced6bd1f0365397f35b32a

Pushed by mozilla@christophkerschbaumer.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/f6379ae84275 Ensure History loads pass valid triggeringPrincipal. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/d3b4dbb64632 Store triggeringPrincipal with all history entries. r=mdeboer https://hg.mozilla.org/integration/mozilla-inbound/rev/eaf9152703d8 Ensure tests performing a history load pass valid triggeringPrincipal. r=bz,mdeboer

https://hg.mozilla.org/mozilla-central/rev/f6379ae84275 https://hg.mozilla.org/mozilla-central/rev/d3b4dbb64632 https://hg.mozilla.org/mozilla-central/rev/eaf9152703d8

Tanvi, I think we fixed this bug earlier than we thought we would. Since it's early in the cycle we could uplift the changes. What do you think? If you want to have it uplifted, could you please fill out the form - thanks!

Comment on attachment 8829576 [details] [diff] [review] bug_1307736_assert_history_loads_pass_valid_triggeringPrincipal.patch Approval Request Comment [Feature/Bug causing the regression]: 1182569 - convert docshell to use AsyncOpen2 [User impact if declined]: Potential for security bugs. With a missing triggeringPrincipal, we fall back to systemPrincipal. Falling back to systemPrincipal means we skip various security checks. This bug ensures that we pass valid triggeringPrincipals. [Is this code covered by automated tests?]: Tests included in this bug [Has the fix been verified in Nightly?]: No; I'm not sure how to verify this as its not user facing. [Needs manual test from QE? If yes, steps to reproduce]: No [List of other uplifts needed for the feature/fix]: [Is the change risky?]: Only in that it adds an assertion which could fail in debug builds. But the failures are what we want to capture and debug in order to find potential security issues. [Why is the change risky/not risky?]: See above. [String changes made/needed]: None

Comment on attachment 8830855 [details] [diff] [review] bug_1307736_update_session_management.patch Approval Request Comment See comment 58.

Comment on attachment 8830856 [details] [diff] [review] bug_1307736_test_updates.patch Approval Request Comment See comment 58.

Modified from Uplift Dashboard.

Comment on attachment 8829576 [details] [diff] [review] bug_1307736_assert_history_loads_pass_valid_triggeringPrincipal.patch Given the user impact, let's take it in Aurora53

https://hg.mozilla.org/releases/mozilla-aurora/rev/f27e061cee9d https://hg.mozilla.org/releases/mozilla-aurora/rev/664f2764b2be https://hg.mozilla.org/releases/mozilla-aurora/rev/721ea96b25ca

I'm hitting this on a nightly debug check: Assertion failure: triggeringPrincipal (need a valid triggeringPrincipal to load from history), at /home/worker/workspace/build/src/docshell/base/nsDocShell.cpp:12592 I'm now running debug since https://bugzilla.mozilla.org/show_bug.cgi?id=1331414 makes it possible to have not too many slowdowns by setting javascript.options.jit.full_debug_checks to false. Is this important to report? What should I include in the report?

To give more context. I'm hitting this consistently for pinned tabs.

(In reply to Hannes Verschore [:h4writer] from comment #65) > To give more context. I'm hitting this consistently for pinned tabs. Hannes, can you please file a new bug blocking this one with some STRs and assign the bug to me? I am pretty busy right now but I am trying to have a look as soon as possible. Thanks!

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #66) > (In reply to Hannes Verschore [:h4writer] from comment #65) > > To give more context. I'm hitting this consistently for pinned tabs. > > Hannes, can you please file a new bug blocking this one with some STRs and > assign the bug to me? I am pretty busy right now but I am trying to have a > look as soon as possible. Thanks! Looks bug 1341589 has been filed for this.

(In reply to Hsin-Yi Tsai (55 Regression Engineering support) [:hsinyi] from comment #67) > Looks bug 1341589 has been filed for this. Indeed. Clearing my ni? queue at the moment, hence also clearing this ni?.