Browsing with Fission and session history in parent enabled, I ran into:
TypeError: can't access property "originAttributes", this.browser.contentPrincipal is null in SessionStore.jsm:1030:11

Investigating.

With Fission, for contentPrincipal to be null, we need browser._contentPrincipal to be null.

This could happen as follows:

• when swapping with another docshell with a null browser._contentPrincipal;
• if nsScriptSecurityManager::GetLoadContextContentPrincipal returns null, which cannot happen (it throws instead);
• if BrowserParent::RecvOnLocationChange gets passed an empty content principal, which cannot happen (the type is not nullable);
• after a resetFields, which happens only during construction/destruction.

Mike, do you know if we could have some kind of race condition between browser construction/destruction and session history?

I think that during swapping docShells is your best bet here, since we kick off things when we get the SwapDocShells event.

Thanks.

Reading through the code I don't think that this can happen, so I'm currently stumped. What would happen if we simply ignored notifySHistoryChanges (or maybe just this block) if this.browser.contentPrincipal was null?

It might be helpful to have an example of something that's actually causing this before we patch it. The block you linked to is only used to cache browser properties in case we don't have a browser when doing the actual flush here. Bug 1572084 will hopefully make it so we always have a browser at that point. That work isn't landing soon, but it might be fine to just ignore this error until it does. I'm mostly just afraid that the patch will cause us to start using stale cached data in session store flushes.

(In reply to :kashav from comment #5)

It might be helpful to have an example of something that's actually causing this before we patch it. The block you linked to is only used to cache browser properties in case we don't have a browser when doing the actual flush here. Bug 1572084 will hopefully make it so we always have a browser at that point. That work isn't landing soon, but it might be fine to just ignore this error until it does. I'm mostly just afraid that the patch will cause us to start using stale cached data in session store flushes.

I haven't been able to reproduce or find a codepath that leads to this state. It doesn't look grave, though, so, if it's covered by your work, we should be ok.

Haven't seen this recently but keeping it for now for fission-history in case it is reproduced again. I'll try to get STR next time I see this.

Kashav, I've applied Nika's suggestion. What test can I run to make sure I go through any of this codepath?

We have a few sessionstore tests that check userContextId, but I'm not sure if we have anything that test the fallback _lastKnown* values (they were added in this patch, peterv may have a better idea).

I saw this error again in my browser console today.

We shouldn't need to get the contentPrincipal to determine the userContextId as that property is actually required to be specified by frontend code in an attribute. We can probably get away with instead checking browser.getAttribute("usercontextid") || 0 like we do in a few other places (e.g. https://searchfox.org/mozilla-central/rev/0b2870194375d80b54608c39060884acb758c206/browser/base/content/browser.js#6952).

That being said, the entire setup is a bit weird, as it's not possible for the usercontextid of a browser to change over time, so it seems odd that we're recording a "last known value" like it's going to change on us.

Tom, can you take this one up? Please reach out to Nika for questions or clarifications.

I agree that we should change the way of getting userContextId. It shouldn't be possible to change over time.

Checking the use of _lastKnownUserContextId and it's only be used at here. And I have a patch(D98468) removes the only use of _lastKnownUserContextId [1]. Once D98468 is landed, I think we can even remove all the code for collecting _lastKnownUserContextId from the content principal since they are not needed.

However, D98468 hasn't been landed because it would cause browser_637020.js to fail after applying that patch. It looks like D98468 would reveal an issue that TAB_STATE_FOR_BROWSER is not handled properly while restoring windows. I submitted a patch for that and put my understanding at https://phabricator.services.mozilla.com/D100249#3258624. Unfortunately, D100249 hasn't been reviewed yet.

[1] https://searchfox.org/mozilla-central/search?q=symbol:%23_lastKnownUserContextId&redirect=false

https://hg.mozilla.org/mozilla-central/rev/729a2e07309f