608191 - "ASSERTION: EnsureReflowFlushAndPaint() called with no docshell!" with prompt()

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: EnsureReflowFlushAndPaint() called with no docshell!: 'mDocShell', file dom/base/nsGlobalWindow.cpp, line 4298

Comment 1

[Jesse Ruderman]

nsGlobalWindow::EnsureReflowFlushAndPaint [dom/base/nsGlobalWindow.cpp:4300]
nsGlobalWindow::Prompt [dom/base/nsGlobalWindow.cpp:4584]
NS_InvokeByIndex_P
...

Comment 2

[Boris Zbarsky [:bzbarsky]]

Uh.... that assert is bogus, no?  Or the callsite needs to be checking for a docshell.

Comment 3

[Nochum Sossonko [:Natch]]

(In reply to comment #2)
> Uh.... that assert is bogus, no?  Or the callsite needs to be checking for a
> docshell.

Yes, it's bogus. See bug 623447. We bail anyhow the same way in ::GetTop, where we anyhow check if there is no docshell. These need to be fixed.

Comment 4

[Nochum Sossonko [:Natch]]

(In reply to comment #3)
> (In reply to comment #2)
> > Uh.... that assert is bogus, no?  Or the callsite needs to be checking for a
> > docshell.
> 
> Yes, it's bogus. See bug 623447. We bail anyhow the same way in ::GetTop, where
> we anyhow check if there is no docshell. These need to be fixed.

Wait, scratch that. I'm not sure I understand this. The docshell goes away after the call to GetTop? Is this still happening?

Comment 5

[Nochum Sossonko [:Natch]]

Ok, so I'm thinking this might just be due to the fact that AreDialogsBlocked isn't forwarded to the outer window (as isn't Prompt). So there's probably a docshell on the outer window but not on the inner. Does that make any sense?

Comment 6

[Nochum Sossonko [:Natch]]

(Oh, and GetTop is forwarded, so there's no check on mDocShell for the inner window).

Comment 7

[Boris Zbarsky [:bzbarsky]]

> The docshell goes away after the call to GetTop?

What call to GetTop?

> So there's probably a docshell on the outer window but not on the inner.

This statement is correct.

Comment 8

[Nochum Sossonko [:Natch]]

(In reply to comment #7)
> > The docshell goes away after the call to GetTop?
> 
> What call to GetTop?

In Prompt there's a call to AreDialogsBlocked. In AreDialogsBlocked there is a call to GetTop, which checks if there is a docshell. If there isn't we return null and AreDialogsBlocked returns true which should cause us to bail with an error from Prompt.

> > So there's probably a docshell on the outer window but not on the inner.
> 
> This statement is correct.

Then that seems to be the problem. EnsureReflowFlushAndPaint is not forwarded. In Alert and Confirm that means it's acting on the outer window since Alert and Confirm are both forwarded. With Prompt that means it's acting on the inner window. It won't get caught in GetTop since GetTop is forwarded.

Comment 9

[Nochum Sossonko [:Natch]]

Also, I can't see how this wouldn't happen on 1.9.2, can someone confirm if it happens there as well?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Sounds like we should forward Prompt (and maybe EnsureReflowFlushAndPaint, or just add an assert in it that it's running on the outer window).

Comment 11

[Nochum Sossonko [:Natch]]

Attachment: like so?

Had to add the forwarding to ShowModalDialog too.

Comment 12

[Nochum Sossonko [:Natch]]

This assertion was added in bug 115352 when the function was changed from accepting a docshell as a parameter to using mDochShell. I'm not sure if this is a serious assertion in this case, we don't crash in a release build because we check for mDocShell anyhow and return early. However, that means that we don't flush layout before going into modal mode. Again, in this particular case, I don't know if that's bad.

To clarify this, I'm requesting blocking.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Not doing this particular flush is, iirc, a spoofing vector.

Comment 14

[Nochum Sossonko [:Natch]]

This goes too far back for me, the EnsureReflowFlushAndPaint originated in bug 77002. What kind of spoofing is possible? AFAICT the worry is that some layout values won't be correct (e.g. .width and .height).

Comment 15

[Boris Zbarsky [:bzbarsky]]

Making it look like the prompt is from a different website from the one it's actually from, by posing it during paint suppression.

Comment 16

[Nochum Sossonko [:Natch]]

(In reply to comment #15)
> Making it look like the prompt is from a different website from the one it's
> actually from, by posing it during paint suppression.

I don't see how that can be done with this testcase, however it can be easily done with onunload="prompt()" and the user closing the tab...

Comment 17

[Johnny Stenback (:jst)]

Not blocking.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 502023 [details] [diff] [review]
like so?

Imo, yes.

Comment 19

[Nochum Sossonko [:Natch]]

Pushed: http://hg.mozilla.org/mozilla-central/rev/47cbe0ba759c