312563 - Site crashes camino or breaks window updating (recursive widget updates) [@ nsViewManager::GetDeviceContext]

Status: RESOLVED FIXED

(Camino Graveyard :: Page Layout, defect, P1)

Description

[Simon Fraser [no longer active]]

Forked from bug 308802.
Load the site in the URL (I had java turned off); it crashes, blowing the stack.

Comment 1

[Simon Fraser [no longer active]]

Bad crash, 1.0 material.

Comment 2

[Simon Fraser [no longer active]]

Attachment: Crash log

Comment 3

[Simon Fraser [no longer active]]

Attachment: Patch

This patch fixes the crash (on the branch, at least; I wasn't able to reliably
crash on the trunk).

However, the window is left in a very weird state after visiting this site with
Java turned off. Navigating to new pages doesn't draw properly, news tabs don't
show up, and clicks in various parts of the window are ignored.

Comment 4

[Simon Fraser [no longer active]]

The code that is causing the weird non-refreshing window issue with this page is
the Invalidate(PR_TRUE) call in nsChildView::RemovedFromWindow(). Note that the
view has just been removed from the window, but that synchronous invalidate is
trying to redraw it, so sometimes AppKit throws assertions about -lockFocus
failing. Not surprising.

The reason that the synchronous Invalidate() is there is to cause a painting
cycle on the content, so that plugins get a paint event at tab switch time,
which, in theory, should have a null cliprect and cause them to stop drawing.
This is a total hack, and needs to be fixed the right way (bug 277067).

Comment 5

[Mark Mentovai]

Comment on attachment 199681 [details] [diff] [review]
Patch

These are reasonable checks.

Comment 6

[Simon Fraser [no longer active]]

*** Bug 312594 has been marked as a duplicate of this bug. ***

Comment 7

[Simon Fraser [no longer active]]

I'd love to rip out nsChildView::RemovedFromWindow() and
nsChildView::AddedToWindow(), but it regresses Java applet drawing when
switching tabs (as expected).

Comment 8

[Simon Fraser [no longer active]]

I checked in the patch. However, we still hose cocoa window updates (read up),
so keeping bug open.

Comment 9

[Simon Fraser [no longer active]]

*** Bug 313093 has been marked as a duplicate of this bug. ***

Comment 10

[Simon Fraser [no longer active]]

Attachment: Patch to rip out the "notify plugin" hack

This patch removes the troublesome code. It may regress plugins drawing through
tabs on tab switching (i.e. Camino will now behave similarly to Firefox for bug
277067).

Comment 11

[Mark Mentovai]

Comment on attachment 200228 [details] [diff] [review]
Patch to rip out the "notify plugin" hack

I can't really decide which behavior is worse, but I suppose it's better to let
things bleed through tabs (recoverable) than to kill the window.

Comment 12

[Simon Fraser [no longer active]]

Checked in on trunk and branch.

Comment 13

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

In bug 313127, Mike asked why all the recent widget recursion regressions and
Simon replied "I think something in gecko (the view manager?) changed on the
branch fairly recently."

Using Steven's regression window from bug 308802, bet. 9-30 and 10-04 (several
days without builds, unfortunately), we get bonsai: http://tinyurl.com/apnmh

Among those checkins is bug 281709, which added some stuff to the *View files in
Cocoa widget (a branch-only change, so it doesn't show up in CVS blame, etc., at
least in what appears to be the default presentation).  It looks like bug 281709
caused a bunch of layout regressions and was backed out yesterday in bug 311146
(looks like there's a new fix brewing in bug 281709).

Is there any chance that those changes caused our recursive widget bug (which
seemed to be almost branch-only), and if so, can we put back in the stuff Simon
just ripped out so as to un-regress plugin drawing on tab switches?

Comment 14

[Simon Fraser [no longer active]]

(In reply to comment #13)
> In bug 313127, Mike asked why all the recent widget recursion regressions and
> Simon replied "I think something in gecko (the view manager?) changed on the
> branch fairly recently."
> 
> Using Steven's regression window from bug 308802, bet. 9-30 and 10-04 (several
> days without builds, unfortunately), we get bonsai: http://tinyurl.com/apnmh
> 
> Among those checkins is bug 281709, which added some stuff to the *View files in
> Cocoa widget (a branch-only change, so it doesn't show up in CVS blame, etc., at
> least in what appears to be the default presentation).  It looks like bug 281709
> caused a bunch of layout regressions and was backed out yesterday in bug 311146
> (looks like there's a new fix brewing in bug 281709).
> 
> Is there any chance that those changes caused our recursive widget bug (which
> seemed to be almost branch-only), and if so, can we put back in the stuff Simon
> just ripped out so as to un-regress plugin drawing on tab switches?

Nice analysis. I think you're right. I'll test with the latest changes from bug
281709.

Comment 15

[Simon Fraser [no longer active]]

(In reply to comment #12)
> Checked in on trunk and branch.

Actually it seems that this only made it onto the trunk. So this might explain
bug 313356.

Comment 16

[Simon Fraser [no longer active]]

OK, I checked that last patch into the branch, and it fixes bug 313356. So I
think the best thing to do here is to keep that hack out, and live with the
plugin tab switching regressions, which will eventually get fixed via bug 277067.