1441445 - Investigate aPrincipal assertion failures in nsIOService::SpeculativeConnectInternal

Status: RESOLVED INVALID

(Core :: DOM: Core & HTML, enhancement, P2)

Description

[Andrea Marchesini [:baku]]

This method is called multiple times with a null principal. For instance:

https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsIOService.cpp#1916,1931

Having NS_ASSERTION(aPrincipal) is wrong.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: assertion.patch

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8954274 [details] [diff] [review]
assertion.patch

Review of attachment 8954274 [details] [diff] [review]:
-----------------------------------------------------------------

I like being more aggressive here, but if we are calling it with a null principal quite so many times, how does that pass TRY?

Comment 3

[Andrea Marchesini [:baku]]

Probably we don't have any tests for this scenario. But, changing this NS_ASSERTION to MOZ_ASSERT, after a session restored, the parent process crashes.

Comment 4

[Andrea Marchesini [:baku]]

Right, this patch is the wrong one. I forgot to exec hg qref.

Comment 5

[Andrea Marchesini [:baku]]

Attachment: bug_1441445_get_rid_of_nsioservice_speculativeconnectinternal_aprincipal_as

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8954297 [details] [diff] [review]
bug_1441445_get_rid_of_nsioservice_speculativeconnectinternal_aprincipal_as

Review of attachment 8954297 [details] [diff] [review]:
-----------------------------------------------------------------

As discussed over IRC, we shouldn't remove the assertion, but rather make sure the callsites pass a valid triggeringPrincipal. You mentioned we don't have an automated testcase. Can you add some STRs so we can figure out the actual problem, fix it and add an automated test for it? Thanks!

Comment 7

[Andrea Marchesini [:baku]]

I filed bug 1461930. It seems that SessionStore.jsm is the component that passes a null principal calling SpeculativeConnect instead o SpeculativeConnect2.

Comment 8

[Jonathan Kingston [:jkt] he/him]

Going to close this as the correct solution is fixing bug 1461930 which I am working on.