2009681 - Assertion reached when printing in Thunderbird: Load looks like first load but does not want principal inheritance

Status: NEW

(Core :: DOM: Navigation, defect)

Description

[Kai Engert [:KaiE:]]

In the scenario reported in bug 2009223, which attempts to Print multiple messages in Thunderbird, the following assertion fails in a debug build:
https://searchfox.org/firefox-main/rev/72a543fee7d16f07f6f04bd6caf7c7692ed291bf/docshell/base/nsDocShell.cpp#10711-10713

In release builds, because the assertion check is probably skipped, we crash at other places, for example:

#0 0x00007fffe868a4bd in nsDocumentViewer::SetBoundsWithFlags(mozilla::gfx::IntRectTyped<mozilla::LayoutDevicePixel> const&, unsigned int) () from /home/user/local/tb-daily/thunderbird/libxul.so
#1 0x00007fffe8961d6e in nsDocShell::SetPositionAndSize(int, int, int, int, unsigned int) ()
from /home/user/local/tb-daily/thunderbird/libxul.so
#2 0x00007fffe87c120c in nsSubDocumentFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) () from /home/user/local/tb-daily/thunderbird/libxul.so
#3 0x00007fffe87ad496 in nsLineLayout::ReflowFrame(nsIFrame*, nsReflowStatus&, mozilla::ReflowOutput*, bool&) ()
from /home/user/local/tb-daily/thunderbird/libxul.so
#4 0x00007fffe870cd60 in nsBlockFrame::ReflowInlineFrame(mozilla::BlockReflowState&, nsLineLayout&, GenericLineListIterator<nsLineLink, false>, nsIFrame*, LineReflowStatus*) () from /home/user/local/tb-daily/thunderbird/libxul.so

Comment 1

[Kai Engert [:KaiE:]]

Attachment: stack.txt

Full stack attached.

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

:vhilla, since you are the author of the regressor, bug 543435, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 3

[Vincent Hilla [:vhilla]]

https://pernos.co/debug/KfCtJSinpRnAHvlBWDb92g/index.html

I think this JS stack from createContentWindow to setting openWindowInfo is relevant:

0 createPreviewBrowser(sourceVersion = ""source"") ["chrome://global/content/printUtils.js":767:7]
    this = [object XULElement]
1 createParentBrowserForStaticClone(aBrowsingContext = "[object CanonicalBrowsingContext]", aOpenWindowInfo = "[xpconnect wrapped nsIOpenWindowInfo @ 0x7006bf636b20 (native @ 0x7006e36b5fa0)]") ["chrome://global/content/printUtils.js":412:32]
    this = [object Object]
2 handleStaticCloneCreatedForPrint(aOpenWindowInfo = "[xpconnect wrapped nsIOpenWindowInfo @ 0x7006bf636b20 (native @ 0x7006e36b5fa0)]") ["chrome://global/content/printUtils.js":399:17]
    this = [object Object]
3 getContentWindowOrOpenURI(aURI = "null", aOpenWindowInfo = "[xpconnect wrapped nsIOpenWindowInfo @ 0x7006bf636b20 (native @ 0x7006e36b5fa0)]", aWhere = "4", aFlags = "0", aTriggeringPrincipal = "[xpconnect wrapped nsIPrincipal @ 0x7006bf636b80 (native @ 0x7006b392d380)]", aCsp = "null", aSkipLoad = "true") ["chrome://messenger/content/mailWindow.js":841:20]
    this = [object Object]
4 createContentWindow(aURI = "null", aOpenWindowInfo = "[xpconnect wrapped nsIOpenWindowInfo @ 0x7006bf636b20 (native @ 0x7006e36b5fa0)]", aWhere = "4", aFlags = "0", aTriggeringPrincipal = "[xpconnect wrapped nsIPrincipal @ 0x7006bf636b80 (native @ 0x7006b392d380)]", aCsp = "null") ["chrome://messenger/content/mailWindow.js":776:17]
    this = [object Object]
5 anonymous() ["chrome://messenger/content/about3Pane.js":7498:53]
6 AsyncFunctionNext(val = "undefined") ["self-hosted":780:27]
    this = [object Object]

From a first look:

• PrintUtils.createParentBrowserForStaticClone creates a browser with no src, we load an initial about:blank. This is a system context, we use a system principal as trigger. But it's a content browser, so nsDocShellLoadState::SetupInheritingPrincipal uses a new null principal for the document. This effectively means we don't want principal inheritance.
• But we create the initial about:blank eagerly when creating the docshell. And there, nsFrameLoader::mOpenWindowInfo provides a content principal to be used. So once the load occurs, we'll hit an assert.
• This nsIOpenWindowInfo comes from nsWindowWatcher::OpenWindowInternal and uses nsContentUtils::SubjectPrincipal() as principal to inherit. In Firefox, this subject principal would also be used as triggering principal later when nsWindowWatcher calls LoadURI. But ThunderBird has it's own nsIWindowProvider which somehow calls into PrintUtils and triggers the load before nsWindowWatcher.

We should probably assert somewhere that the triggering principal matches mOpenWindowInfo. Either in ReallyStartLoadingInternal or nsFrameLoader::LoadURI.

But what is the correct principal here? And could the print preview avoid the about:blank load?

Comment 4

[Vincent Hilla [:vhilla]]

(In reply to Vincent Hilla [:vhilla] from comment #3)

In Firefox, this subject principal would also be used as triggering principal later when nsWindowWatcher calls LoadURI.

Oh, nsGlobalWindowOuter::Print leaves the URI empty so no load would occur in Firefox.

The primary issue is that bug 543435 changed nsIOpenWindowInfo to contain mPrincipalToInheritForAboutBlank. Thereby, we now implicitly specify which principal ProvideWindow should use. I think Thunderbird's window provider needs to be adapted.

We could

• have createPreviewBrowser set nodefaultsrc to true
• or change how nsFrameLoader::MaybeCreateDocShell uses mOpenWindowInfo, because this wasn't previously used to propagate principals from JS to Gecko
• or somehow change what is used as triggering principal either via an attribute on browser or by modifying nsFrameLoader::LoadFrame.

---

But that doesn't explain bug 2009223. Because if I set nodefaultsrc, I get a crash due to the viewer's mPresShell being null: https://pernos.co/debug/VKt6A_ZKOrffRXwVgHzF6A/index.html

The same happens if I change nsFrameLoader::MaybeCreateDocShell to ignore mOpenWindowInfo for determining the principal and disable an assert for that principal in nsWindowWatcher.

It's still possible that mPresShell being null is caused by bug 543435 through some timing change, but it's less certain. SetPrintPreviewPresentation or InitPresentationStuff wasn't called.

Comment 5

[Vincent Hilla [:vhilla]]

nsPrintJob::Print sets aIsPrintPreview=false and so ReflowPrintObject won't call mDocViewerPrint->SetPrintPreviewPresentation. InitPresentationStuff returns early due to mPrintJob. ReflowPrintObject calls doc->CreatePresShell and I don't think this is ever passed to the viewer.

:emilio, based on D88901 we prevent print documents from getting a non-print presentation. Should something have blocked nsDocumentViewer::Show for the print doc? Could sync-about-blank (bug 543435) have broken this?

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 543435

Comment 7

[Emilio Cobos Álvarez [:emilio]]

Not sure what the question is... Broken what exactly? The principal inheritance thing? If so, sure looks suspicious. Printing in Firefox doesn't seem broken tho, right? The stack in createParentBrowserForStaticClone looks normal... Why are we triggering a load even when we use nodefaultsrc="true"? My understanding is that that is intended to prevent the initial load: https://searchfox.org/firefox-main/rev/6ece603789f6751c37c48b23f39dbbb16b290592/dom/base/nsFrameLoader.cpp#510-522

Can you provide more context on how Show() is involved here? So far all you've described in comment 5 seems reasonable. The reflow is managed explicitly from nsPrintObject for actual printing, but not for print preview.

Comment 8

[Emilio Cobos Álvarez [:emilio]]

The reflow happens around here ftr.

Comment 9

[Vincent Hilla [:vhilla]]

The question is why is there a null ptr access on nsDocumentViewer::mPresShell when printing (a) in opt builds and (b) when I try fixing this "Load looks like first load but does not want principal inheritance" assertion by setting nodefaultsrc on the print browser. I wonder whether this is a second issue on top of the principal inheritance that causes non-nsPrintObject code to reflow the print document.

https://searchfox.org/firefox-main/rev/6ece603789f6751c37c48b23f39dbbb16b290592/layout/printing/nsPrintJob.cpp#1327-1330

nsresult nsPrintJob::ReflowPrintObject() {
  ...
  if (mIsCreatingPrintPreview && documentIsTopLevel) {
    mDocViewerPrint->SetPrintPreviewPresentation(aPO->mPresContext,
                                                 aPO->mPresShell.get());
  }

https://searchfox.org/firefox-main/rev/6ece603789f6751c37c48b23f39dbbb16b290592/layout/base/nsDocumentViewer.cpp#662-666

nsresult nsDocumentViewer::InitPresentationStuff() {
  ...
  if (mPrintJob) {
    return NS_OK;
  }

This code looks like we don't want to have a PresShell on nsDocumentViewer for non-preview print. But FlushPendingNotifications for chrome://messenger/content/messenger.xhtml causes calls to the print document viewer. Show() early-returns and SetBoundsWithFlags() crashes.

Comment 10

[Emilio Cobos Álvarez [:emilio]]

(In reply to Vincent Hilla [:vhilla] from comment #9)

This code looks like we don't want to have a PresShell on nsDocumentViewer for non-preview print.

That's correct.

But FlushPendingNotifications for chrome://messenger/content/messenger.xhtml causes calls to the print document viewer. Show() early-returns and SetBoundsWithFlags() crashes.

How? Do you have a pernosco link or so? I guess that means the print document is same-process with messenger.xhtml, but still, how does flushing end up flushing the print doc? It shouldn't be in the same browsing context tree...

Actually while writing this comment, that made me ask the question: what's different between what TB is doing and printing about:config (or any other same process page). And indeed with print preview disabled (print.always_print_silent = true), printing about:config crashes, and mozregression points to bug 2000626. So I think we have a root cause / repro :)