420415 - Crash while quitting with dialog from <xul:iframe src="nosuchprotocol:">

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect, P1)

Description

[Jesse Ruderman]

About 10% of the time, this assertion failure occurs just before the dialog appears:

###!!! ASSERTION: painting in the middle of frame construction: 'mPresContext->mLayoutPhaseCount[eLayoutPhase_FrameC] == 0', file ../../dist/include/layout/nsPresContext.h, line 931

If I click "OK" in the dialog, nothing bad happens.  But if I press Cmd+Q while the dialog is up, there is a 60% chance of crashing, regardless of whether the assertion failure occurred.  The crash dereferences 0xdddddde9.

Comment 1

[Jesse Ruderman]

Attachment: testcase

Comment 2

[Jesse Ruderman]

Attachment: stack traces

Comment 3

[Boris Zbarsky [:bzbarsky]]

Uh, this is really bad.  We're putting up a modal dialog in the middle of frame construction.

Luckily, smaug's patch should fix that part.  But then we'd still be putting up a modal dialog during BindToTree.

Can we make that modal dialog async or something?  Or do the load or the DisplayLoadError call async?

Comment 4

[Jesse Ruderman]

Smaug's patch in which bug?

Comment 5

[Martijn Wargers (dead)]

Works for me, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre) Gecko/2008022906 Minefield/3.0b4pre

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

We need to do the same thing for loadFrame as what was done for Destroy.

Comment 7

[Daniel Veditz [:dveditz]]

on the 1.8 branch I see the assertion

###!!! ASSERTION: failed to load URL: 'NS_SUCCEEDED(rv)', file c:/dev/ff2/mozilla/content/base/src/nsFrameLoader.cpp, line 183

and then crash dereferencing 0xdddddde9 in nsCachedStyleData::GetStyleData()

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Attachment: this should fix this (with the patch for 395609)

Have to still test some more. I can't reproduce the crash, but without the patches I did see the assertion.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

At least with the patches I can't see the assertion, which should mean
that frame loading starts at a safe time.
I did run also mochitest/browser/chrome, everything looks ok.

Comment 10

[Damon Sicore (:damons)]

Who's reviewing the patch?

+'ing w/P2.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Have to solve the window-event-loop problem in bug 395609 first.
But I think jonas and jst should review, since they reviewed Bug 402982.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

And btw, if anyone on mac could test the patch and the latest patch for bug 395609 to see if the crash is fixed ;)

(I *must* get a mac, unfortunately.)

Comment 13

[Jesse Ruderman]

I'll test this patch once the patch for bug 395609 goes in.

Comment 14

[Damon Sicore (:damons)]

Fixing flags to reflect blocking.  +'ing w/ P2.  

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 307451 [details] [diff] [review]
this should fix this (with the patch for 395609)

Ugh, sorry, could have sworn i marked this a while ago. Sorry about the delay.

Comment 16

[Mike Beltzner [:beltzner, not reading bugmail]]

TM --> mozilla1.9, this won't block beta 5. If you disagree, please reset the TM to beta5 and explain why it needs to block beta.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Bug 421209, Bug 395609 and this one should, IMO, go in together.
Bug 395609 is by far the most scariest one, but this one may cause regressions 
too. I'd really like to get these all in for beta5.

Bug 421209 blocks Bug 395609, which blocks this one.

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 307451 [details] [diff] [review]
this should fix this (with the patch for 395609)

a=beltzner

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Attachment: with PRInt32 -> PRUint32

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Checked in.

Comment 21

[Daniel Veditz [:dveditz]]

This testcase gives me a potentially exploitable crash on branch (comment 7) but later comments say this fix depends on bug 395609 which isn't a problem on the 1.8 branch (bug 395609 comment 81).

Is the branch crash a different bug, or do we still need this chain of fixes even though some of the other testcases are masked on the branch?

Comment 22

[Daniel Veditz [:dveditz]]

Another potential regression found (bug 431082) so we need to wait until we get those sorted out.