1666584 - Dereferencing this.browser.contentPrincipal without NULL check

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect, P3)

Description

[Neha Kochar [:neha]]

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

Comment 1

[David Teller [:Yoric] - still alive but not very active]

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?

Comment 2

[Mike de Boer [:mikedeboer]]

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

Comment 3

[David Teller [:Yoric] - still alive but not very active]

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?

Comment 4

[David Teller [:Yoric] - still alive but not very active]

Attachment: Bug 1666584 - Don't add a Session History listener if we don't have a content principal;r?mikedeboer

Comment 5

[u608768]

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.

Comment 6

[David Teller [:Yoric] - still alive but not very active]

(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.

Comment 7

[Neha Kochar [:neha]]

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.

Comment 8

[David Teller [:Yoric] - still alive but not very active]

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

Comment 9

[u608768]

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).

Comment 10

[Neha Kochar [:neha]]

I saw this error again in my browser console today.

Comment 11

[Nika Layzell [:nika] (ni? for response)]

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).

Comment 12

[Nika Layzell [:nika] (ni? for response)]

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.

Comment 13

[Neha Kochar [:neha]]

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

Comment 14

[Tom Tung [:tt, :ttung]]

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

Comment 15

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1666584 - Remove the code for updating _lastKnownUserContextId because it's not needed;

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/729a2e07309f
Remove the code for updating _lastKnownUserContextId because it's not needed; r=nika

Comment 17

[Dorel Luca [:dluca]]

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