Port Bug 1490257 - Add asserts into loadURI where we imply SystemPrincipal - Total debug MozMill failure or 2018-11-01

RESOLVED FIXED in Thunderbird 65.0

Status

enhancement
RESOLVED FIXED
10 months ago
10 months ago

People

(Reporter: jorgk, Assigned: jorgk)

Tracking

(Blocks 1 bug)

Trunk
Thunderbird 65.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Assertion failure: false (LoadURI: System principal required.), at /builds/worker/workspace/build/src/docshell/base/nsDocShellLoadState.cpp:499
OK, I added xpc_DumpJSStack(true,true,true); before the two MOZ_ASSERT()s in nsDocShell.cpp and this way I could identify where principals were missing.

I'll attach WIP patch in a minute.
This is just a start. There are more webNavigation.LoadURI() calls without principal.

Clicking onto a new message I got this crash:
xul.dll!nsDocShellLoadState::SetupTriggeringPrincipal(const mozilla::OriginAttributes & aOriginAttributes) Line 499	C++
xul.dll!nsDocShell::LoadURI(nsDocShellLoadState * aLoadState) Line 714	C++
xul.dll!nsNntpService::GetMessageFromUrl(nsIURI * aUrl, nsIMsgWindow * aMsgWindow, nsISupports * aDisplayConsumer) Line 319	C++
xul.dll!nsNntpService::DisplayMessage(const char * aMessageURI, nsISupports * aDisplayConsumer, nsIMsgWindow * aMsgWindow, nsIUrlListener * aUrlListener, const char * aCharsetOverride, nsIURI * * aURL) Line 291	C++
xul.dll!nsMessenger::OpenURL(const nsTSubstring<char> & aURL) Line 470	C++

so the nsDocShell::LoadURI() calls must somehow also be equipped with a principal. Not sure how yes.
Assignee: nobody → jorgk
Flags: needinfo?(ckerschb)
Fixed more webNavigation.LoadURI() calls. Loading of message still doesn't work, clicking a mailbox message crashes in:
#01: nsMailboxService::FetchMessage (c:\mozilla-source\comm-central\comm\mailnews\local\src\nsMailboxService.cpp:246)
#02: nsMailboxService::DisplayMessage (c:\mozilla-source\comm-central\comm\mailnews\local\src\nsMailboxService.cpp:279)
#03: nsMessenger::OpenURL (c:\mozilla-source\comm-central\comm\mailnews\base\src\nsMessenger.cpp:470)

which is |rv = docShell->LoadURI(loadState);|.
Attachment #9021721 - Attachment is obsolete: true
Comment on attachment 9021750 [details] [diff] [review]
1503735-add-principal-to-loadURI.patch - JS part (v2)

This should do it for the JS part, I'll attach the C++ part in a minute.
Attachment #9021750 - Attachment description: 1503735-add-principal-to-loadURI.patch - WIP v2 → 1503735-add-principal-to-loadURI.patch - JS part (v2)
Flags: needinfo?(ckerschb)
Attachment #9021750 - Flags: review?(mkmelin+mozilla)
Attachment #9021750 - Flags: feedback?(ckerschb)
OK, so I equipped all
webNavigation.loadURI(), browser.loadURI() and nsIDocShell.loadURI() calls with a system principal which was previously implied. With the two patches, TB will start and I can view articles.

Note that browser.loadURI() in mail/components/extensions/ExtensionPopups.jsm and mail/components/extensions/parent/ext-tabs.js already provided principals.

Also, webNav->LoadURI() calls in nsMessenger.cpp, nsMsgPrintEngine.cpp and nsMsgWindow.cpp already provided (system) principals.

I'm not sure about nsIExternalProtocolService.loadURI() calls of which we have a few as well.
Attachment #9021760 - Flags: review?(mkmelin+mozilla)
Attachment #9021760 - Flags: feedback?(ckerschb)
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/d5940f87ebd3
Port bug 1490257: Add system principal to loadURI() calls, JS part. rs=bustage-fix
https://hg.mozilla.org/comm-central/rev/b0459a76e420
Port bug 1490257: Add system principal to loadURI() calls, C++ part. rs=bustage-fix
Status: NEW → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 65.0
Comment on attachment 9021750 [details] [diff] [review]
1503735-add-principal-to-loadURI.patch - JS part (v2)

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

Looks good afaikt, r=mkmelin
Attachment #9021750 - Flags: review?(mkmelin+mozilla) → review+
Attachment #9021760 - Flags: review?(mkmelin+mozilla) → review+
Yes, the push is green.
Comment on attachment 9021750 [details] [diff] [review]
1503735-add-principal-to-loadURI.patch - JS part (v2)

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

Ultimately we should re-visit those changes. In particular, whenever a URI to be loaded can be influenced by the web we should not use the SystemPrincipal. E.g. the file within convbrowser.xml is a static chrome: URI, in that case it's totally fine to use SystemPrincipal. In other cases, e.g. editor.js, I am not so sure if that URL to be loaded can be influenced by the web.

Anyway, in all the cases where the explicit referrer value was null before, it's fine to explicitly pass the SystemPrincipal because it's not making the code any worse than it was before we landed the assertion.
Attachment #9021750 - Flags: feedback?(ckerschb) → feedback+
Comment on attachment 9021760 [details] [diff] [review]
1503735-add-principal-to-loadURI-cpp.patch

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

Similar concerns that I raised within my previous comment also apply to this patch. Given there is no referrer value currently it's not making the code any worse, but it's also not as future proof as I would like it to be. If possible we should re-visit those changes.
Attachment #9021760 - Flags: feedback?(ckerschb) → feedback+
Blocks: 1504670
Thanks Christoph, I've filed bug 1504670 for the follow-up.
You need to log in before you can comment on or make changes to this bug.