Currently, SessionStore.jsm doesn't pass the principal to nsIOService::SpeculativeConnect: https://searchfox.org/mozilla-central/rev/00dd116638001772fe354b081353b73f1cad405d/browser/components/sessionstore/SessionStore.jsm#3576 This means that we assert here: https://searchfox.org/mozilla-central/rev/00dd116638001772fe354b081353b73f1cad405d/netwerk/base/nsIOService.cpp#1840 and, more important, we end up using system principal here: https://searchfox.org/mozilla-central/rev/00dd116638001772fe354b081353b73f1cad405d/netwerk/base/nsIOService.cpp#1842-1846

Gijs, do you know who works on this component? It seems we don't enough tests to trigger this assertion. Plus, I don't know which component this bug should belong to.

(In reply to Andrea Marchesini [:baku] from comment #1) > Gijs, do you know who works on this component? It seems we don't enough > tests to trigger this assertion. > Plus, I don't know which component this bug should belong to. I'm not sure about the question. In case you mean session restore, :mikedeboer. In case you mean DOM and principals, :ckerschb would be my bet...

(In reply to :Gijs (he/him) from comment #2) > (In reply to Andrea Marchesini [:baku] from comment #1) > > Gijs, do you know who works on this component? It seems we don't enough > > tests to trigger this assertion. > > Plus, I don't know which component this bug should belong to. > > I'm not sure about the question. In case you mean session restore, > :mikedeboer. In case you mean DOM and principals, :ckerschb would be my > bet... I don't have a strong opinion what component this bug is in, feel free to move it into DOM:Security. Anyway, we should not fall back to using the SystemPrincipal in that case. Ideally we should try to pass the principal that should be used. Additionally the fallback should rather be a NullPrincipal instead of the Systemprincipal.

Looking at this again, I think it would be beneficial to deprecate and remove: * speculativeConnect(), as well as * speculativeAnonymousConnect() and have all our code be updated to use speculativeConnect2() and speculativeAnonymousConnect2() respectively, passing the right principal [2]. Jonathan, this pretty much aligns with your project, hence I am assigning this one to you too :-) I am marking it a P2, but it could also be handled as a P3! [1] https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsISpeculativeConnect.idl#31-43

Looking at this again as I think this crash might prevent me landing the code I need. Messing with try to see where we are: https://treeherder.mozilla.org/#/jobs?repo=try&revision=877d4c96a609978c7f5a4e4b2a173984517a795a :mikedeboer I think we can't regress the performance of session restore here on tab hover righr? So we might actually have to serialize the principal into TabStateCache and also into the getLazyTabValue store also. Does that sound like a resonable solution? My hunch is we aren't doing the right thing for first party isolation and other cases in this file anyway. We are manually recreating tabs with the userContextId and it seems to me this speculative connect needs to match the whole origin attribute set. Serializing the old triggeringPrincipal of the load would provide the correct details in here.

It looks like the code is all working by using the contentPrincipal of the tab. The question still stands if this will cause a bad perf regression though. Perhaps we should just try to land and see if it sticks; we could file a follow up for doing Comment 5 in another bug.

(In reply to Jonathan Kingston [:jkt] from comment #5) > :mikedeboer I think we can't regress the performance of session restore here > on tab hover righr? So we might actually have to serialize the principal > into TabStateCache and also into the getLazyTabValue store also. Does that > sound like a resonable solution? Well, considering that the serialized principals are pretty large and that shows up in bugs like bug 1287906. I think there's a route to fix that specific case, though, but we should be careful of our memory footprint here. > My hunch is we aren't doing the right thing for first party isolation and > other cases in this file anyway. We are manually recreating tabs with the > userContextId and it seems to me this speculative connect needs to match the > whole origin attribute set. Serializing the old triggeringPrincipal of the > load would provide the correct details in here. I think _correctness_ is most important here, right? The performance concern is OK, but if doing a correct speculative connect on tab hover is becoming too expensive, we should just yank that functionality out, in favor of being correct.

Hey :tjr, Based on Comment 7 it seems like we should at least gate this feature on FPI and maybe just check the userContextId for now. Any thoughts?

(In reply to Jonathan Kingston [:jkt] from comment #8) > Hey :tjr, > Based on Comment 7 it seems like we should at least gate this feature on FPI > and maybe just check the userContextId for now. > > Any thoughts? I thought Comment 7 was saying "Do it correctly, and if it's slow we just won't do it"? Anyway, when we *actually* have to make a connection for a URI (not just speculatively create one) and FPI is turned on, won't we look for an existing connection we can reuse - find none because we'll be looking _with_ FPI attributes and the speculative connection omitted them - and then not use the speculative connection and make one from scratch? (I'm not sure where things like cookies would get queried from, but if they were queried from the cookiejar using the properties of the speculative connection: you might not find any cookies if you had FPI enabled.) When you say "gate this feature on FPI and maybe just check the userContextId for now" - do you mean "Do something special for containers, ignore OriginAttributes, and stick a 'if(fpi_enabled) { return; }' in there"? That sounds kind of hacky (and we should file a followup bug to remove it) but... alright...

(In reply to Tom Ritter [:tjr] from comment #9) > I thought Comment 7 was saying "Do it correctly, and if it's slow we just > won't do it"? Indeed, but I'm not saying it won't be worth the effort even if there's a perf hit :) So a tab hover is a strong indication that the tab will be selected and that we'll be loading/ restoring the tab's content very soon. It'd be good to be able to kick a few gears already in motion, anticipating this action. This is what the code here is trying to accomplish. However, it not doing the right - or perhaps even wasteful - thing for container tabs is not a good state. Therefore, I'd rather be correct with a slight perf hit than any other way.

Just a few notes: *) Regarding cookies, I think what Tom said is correct and we wouldn't reuse the warmed up connection but would rather throw it away. *) Looking through the implementation of SpeculativeConnect2() we should have an assertion that we indeed pass a non null principal and then also remove that insecure fallback to using the systemPrincipal here: [1] https://searchfox.org/mozilla-central/source/netwerk/base/nsIOService.cpp#1775 *) Same holds true for SpeculativeAnonymousConnect which we should remove and replace with SpeculativeAnonymousConnect2().

Please bear in mind that this is an optimization for the general case, i.e. applying to the vast majority of tabbrowser connections. I think it's fine to miss an occasional warmed-up connection under certain conditions (like cookies, post data perhaps?), but it would be nice to have the general case working for container tabs as well.

Not sure why phab wasn't linked here.

Pushed by jkingston@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ccf9864ee59f Replace use of speculativeConnect methods for variants that pass a triggeringPrincipal. r=ckerschb