Bug 1334875 seems intermittent and hard to reproduce. Completely backing out Bug 1307736 seems not like a good way forward. After discussing with Tanvi over IRC we both think the best way forward is to keep the assertion within LoadHistoryEntry within Bug 1307736 but revert to the old semantics and still fall back to the SystemPrincipal if triggeringPrincipal is a nullptr. [1] https://bugzilla.mozilla.org/attachment.cgi?id=8829576&action=diff

Tanvi, Boris, as explained in comment 0, we could make the hard fail within LoadHistoryEntry a soft fail. Which means, we keep the assertion but fall back to using the SystemPrincipal for history loads not having a triggeringPrincipal. Which "basically" means we temporarily go back to using the old semantics before Bug 1307736 landed [1]. To make sure we are on the same page, I hope that fixes the problem. I would imagine that if the history load would have a valid principal and the history load would be blocked because of security checks, then we would receive an error message in the console, which apparently is not happening. Hence I think the problem must be that we return an error from LoadHistoryEntry if we don't receive a valid principal. Bug 1334875 seems intermittent and hard to reproduce. Kamil is going to take a look at Bug 1334875 and tries to find reliable STRs. If we can find STRs we can still try to get the actual problem fixed and uplifted. As a first measure I propose to get this patch landed. What do you think? If Kamil can't find any STRs and you don't feel comfortable landing this patch, I suppose we have to bite the bullet and back out Bug 1307736. [1] https://bugzilla.mozilla.org/attachment.cgi?id=8829576&action=diff [2] https://bugzilla.mozilla.org/show_bug.cgi?id=1334875#c12

Comment on attachment 8834724 [details] [diff] [review] bug_1337622_fall_back_systemprincipal_history_loads.patch r=me. Thank you for picking this up!

Comment on attachment 8834724 [details] [diff] [review] bug_1337622_fall_back_systemprincipal_history_loads.patch Let's try hard to find the root cause of this and stop falling back to systemprincipal in FF 53+.

(In reply to Tanvi Vyas - not reading bugmail, PM me [:tanvi] from comment #3) > Comment on attachment 8834724 [details] [diff] [review] > bug_1337622_fall_back_systemprincipal_history_loads.patch > > Let's try hard to find the root cause of this and stop falling back to > systemprincipal in FF 53+. Yes, I already filed Bug 1338009 to make debugging easier which will help us find the root cause. thanks!

Comment on attachment 8834724 [details] [diff] [review] bug_1337622_fall_back_systemprincipal_history_loads.patch Approval Request Comment [Feature/Bug causing the regression]: Bug 1307736 - Assert history loads pass a valid triggeringPrincipal for docshell loads [User impact if declined]: We haven't identified the root cause yet, but it seems that intermittently some users might not be able to restore sessions (See Bug 1334875 - page not loading on session restoration). [Is this code covered by automated tests?]: Nope, but instead of failing hard if the sessions history entry does not have a valid triggeringPrincipal, we got back to all semantics and fall back to using the systemPrincipal which will *always* allow the history entry to load. [Has the fix been verified in Nightly?]: Not yet - Kamil is on that. As soon as he has verified that, he will post a comment here. [Needs manual test from QE? If yes, steps to reproduce]: Still having trouble to reproduce. See Bug 1334875. [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: No, in fact it reverts a hard fail to a soft fail, which means we keep the MOZ_ASSERT if a history load does not have a valid triggeringPrincipal which will help us to find the root cause, but fall back to the systemPrincipal for the actual load. [Why is the change risky/not risky?]: see above. [String changes made/needed]: no

Kamil, once you have found reliable STRs to reproduce Bug 1334875, can you please verify that this patch fixed the issue?

Pushed by ihsiao@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/39a912717bb6 Temporarily fall back to SystemPrincipal if History entry does not have a valid triggeringPrincipal. r=tanvi,bz

Modified from Uplift Dashboard.

Comment on attachment 8834724 [details] [diff] [review] bug_1337622_fall_back_systemprincipal_history_loads.patch Use old semantics to help find root cause. Aurora53+.

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #6) > Kamil, once you have found reliable STRs to reproduce Bug 1334875, can you > please verify that this patch fixed the issue? Clearing my ni? queue at the moment. Given that no more issues arised in the past 4 months I am clearing my ni? for Kamil.