75661 - Mozilla crashes on some sites if 'alert before downloading' is set

Status: VERIFIED FIXED

(Core :: Networking: Cookies, defect, P1)

Description

[Peter Janes]

If "Alert me before downloading an image" is enabled and "Accept images that
come from the originating server only" is turned on, Mozilla will either hang
(seldom) or crash (regularly).  One of these two behaviours occurs 100% of the time.

Talkbacks TB28991031X and TB28990793K from Linux build 2001041115 exhibit the
behaviour in both a fresh profile and my active profile, respectively.

To duplicate, create a fresh profile, enable "Alert me before downloading an
image" and "Accept images that come from the originating server only", then
navigate to <http://www.mozillazine.org/>.

This has been happening for at least a week, and perhaps two; the last build I
can recall without the problem was 20010328.

Comment 1

[Stephen P. Morse]

Happens on win32 as well.  Changing OS to all.

Prior to the crash there is an assertion failure.  Below are the stack traces 
for both the assertion failure and for the crash.

The assertion failure is at line 819 in nsViewManager.cpp

   NS_ASSERTION(!(PR_TRUE == mPainting), "recursive painting not permitted");

The crash line is at line 1469 in nsViewManager.cpp

   aView->Paint(aRC, aDamageRect, NS_VIEW_FLAG_JUST_PAINT, unused);

and aView is zero at the time of crash.


==============STACK TRACE FOR ASSERTION FAILURE ==============

NTDLL! 77f76274()
nsDebug::Assertion(const char * 0x027358d8, const char * 0x027358c0, const char 
* 0x02735898, int 819) line 286 + 13 bytes
nsViewManager::Refresh(nsIView * 0x04f39470, nsIRenderingContext * 0x05c872b0, 
const nsRect * 0x0012d90c, unsigned int 1) line 819 + 35 bytes
nsViewManager::DispatchEvent(nsViewManager * const 0x04f3ae80, nsGUIEvent * 
0x0012da4c, nsEventStatus * 0x0012d950) line 1913
HandleEvent(nsGUIEvent * 0x0012da4c) line 68
nsWindow::DispatchEvent(nsWindow * const 0x04f38034, nsGUIEvent * 0x0012da4c, 
nsEventStatus & nsEventStatus_eIgnore) line 695 + 10 bytes
nsWindow::DispatchWindowEvent(nsGUIEvent * 0x0012da4c, nsEventStatus & 
nsEventStatus_eIgnore) line 721
nsWindow::OnPaint() line 3825 + 28 bytes
nsWindow::ProcessMessage(unsigned int 15, unsigned int 0, long 0, long * 
0x0012de2c) line 2832 + 17 bytes
nsWindow::WindowProc(HWND__ * 0x003e040e, unsigned int 15, unsigned int 0, long 
0) line 950 + 27 bytes
USER32! 77e7131f()
USER32! 77e71a3d()
NTDLL! 7

============= STACK TRACE FOR CRASH ==================

nsViewManager::PaintView(nsIView * 0x0000000c, nsIRenderingContext & {...}, int 
33686018, int -435480, const nsRect & {...}) line 1469 + 20 bytes
nsViewManager::RenderDisplayListElement(DisplayListElement2 * 0x05baddd0, 
nsIRenderingContext & {...}) line 1423
nsViewManager::RenderViews(nsIView * 0x04d3a8c0, nsIRenderingContext & {...}, 
const nsRect & {...}, int & 0) line 1314
nsViewManager::Refresh(nsIView * 0x04d3a8c0, nsIRenderingContext * 0x063dbeb0, 
const nsRect * 0x0012d90c, unsigned int 1) line 885
nsViewManager::DispatchEvent(nsViewManager * const 0x05cf1530, nsGUIEvent * 
0x0012da4c, nsEventStatus * 0x0012d950) line 1913
HandleEvent(nsGUIEvent * 0x0012da4c) line 68
nsWindow::DispatchEvent(nsWindow * const 0x04d38674, nsGUIEvent * 0x0012da4c, 
nsEventStatus & nsEventStatus_eIgnore) line 695 + 10 bytes
nsWindow::DispatchWindowEvent(nsGUIEvent * 0x0012da4c, nsEventStatus & 
nsEventStatus_eIgnore) line 721
nsWindow::OnPaint() line 3825 + 28 bytes
nsWindow::ProcessMessage(unsigned int 15, unsigned int 0, long 0, long * 
0x0012de2c) line 2832 + 17 bytes
nsWindow::WindowProc(HWND__ * 0x00fe05a6, unsigned int 15, unsigned int 0, long 
0) line 950 + 27 bytes
USER32! 77e7131f()
USER32! 77e71a3d()
NTDLL!

Comment 2

[Stephen P. Morse]

Here are some clues.  No answers yet.  cc'ing danm since this may be the result 
of changes he made recently to dialogs.

What is happening is that we are entering the function Refresh() (actually the 
second overlayded instance of it) to display the image-warning dialog.  This 
dialog is known to be none modal (bug 57188) and results in the following 
warning message:

   WARNING: dependent window created without a parent,
            file y:\mozilla\xpfe\bootstrap\nsWindowCreator.cpp, line 94

While that dialog is up, the rest of the images on the page are being loaded and 
this cause the recursive entry into Refresh().  And that's why we get the 
assertion failure.

If I add code to break out of the routine upon getting the failure, we at least 
don't crash.  But we don't get the page displayed properly either.

Comment 3

[Stephen P. Morse]

Correction:  Above I said

   "the second overlayded instance of it"

Of course I meant to say

   "the second overloaded instance of it"

Comment 4

[Stephen P. Morse]

This might be a dup of bug 75107 -- expecially since they both started happening 
in the past week or two, just about the time that the new imagelib landed.

However I won't mark it a dup just yet and will continue to investigate this one 
a little further.

Comment 5

[Stephen P. Morse]

Since this problem is the result of a recursive entry into refresh, we can avoid 
that by defering the second refresh so that it doesn't occur until after the 
original refresh finishes.  Attaching patch to accomplish this.

I still believe that this crash (regression) is a consequence of the new 
imagelib.  But I don't know what change in imagelib is responsible for this new 
behavior.  And since the fix presented here avoids the crash in a very clean 
manner, it's not worth determining what change is responsible for the crash.

Comment 6

[Stephen P. Morse]

Attachment: patch to avoid recursive entry

Comment 7

[Stephen P. Morse]

cc'ing alecf for super-review.

Comment 8

[Alec Flett]

woah! not my area. cc'ing tor and blizzard? (not sure who else deals with
nsViewManager)

Comment 9

[Kevin McCluskey (gone)]

Note: The patch handles recursive painting when the composite listener is 
responsible for initiating the recursive paint, It may not refresh correctly if 
the recursive paint is initiated from something other than the composite 
listener. When the recursive paint occurs within the composite listener it 
issues a "brute" force repaint of the entire viewmanager window.

I would still consider the recursive painting an ERROR condition. Painting 
should not be initiated while paints are processed, but it's OK for the 
viewmanager to handle this without crashing.

You can check in this change, but please add a NS_WARNING when the recursive 
paint occurs and file a new bug so someone investigates why the recursive paint 
is happening. 

r=[e-mail]


Robert O'Callahan ([e-mail]) would be the best person to super-review 
this bug.

Comment 10

[Kevin McCluskey (gone)]

On second thought, please make the NS_WARNING an NS_ASSERTION, otherwise nobody 
will ever fix the recursive paint problem.

Comment 11

[Stephen P. Morse]

OK, posting same patch as before but with the assertions left in.  This should 
satisfy kevin's objections

Comment 12

[Stephen P. Morse]

Attachment: patch with assertions put back in

Comment 13

[Stephen P. Morse]

Robert O'Callahan, please super-review.  Thanks.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

sr=roc+moz

Comment 15

[Stephen P. Morse]

Fix checked in.

Comment 16

[Stephen P. Morse]

Filed bug 76528 agains kmcclusk for the underlying problem of why the Refresh() 
routine is being entered recursively.

Comment 17

[Tom Everingham]

verified