978001 - make nsViewManager::ProcessPendingUpdatesForView look at the local view manager instead of always the root one

Status: RESOLVED FIXED

(Core :: Web Painting, defect)

Description

[Timothy Nikkel (:tnikkel)]

Attachment: patch

https://tbpl.mozilla.org/?tree=Try&rev=0ba54de38017

Comment 1

[Mats Palmgren (inactive)]

Comment on attachment 8383529 [details] [diff] [review]
patch

># HG changeset patch
>Make nsViewManager::ProcessPendingUpdatesForView look at the local view manager instead of always the root one where it makes sense to do so.

Don't forget to add the bug number.

>-  if (mPresShell && mPresShell->IsNeverPainting()) {
>+  nsIPresShell* presShell = viewManager->mPresShell;
>+  if (presShell && presShell->IsNeverPainting()) {
>     return;

Why not return when the shell is null? Like so:
    if (!presShell || presShell->IsNeverPainting()) {

>       nsAutoScriptBlocker scriptBlocker;
>       NS_ASSERTION(aView->HasWidget(), "Must have a widget!");

Not your fault, but there are two of these assertions with nothing
significant in-between, please remove the one above.

Comment 2

[Timothy Nikkel (:tnikkel)]

(In reply to Mats Palmgren (:mats) from comment #1)
> Don't forget to add the bug number.

Yep, ddded it immediately after filing, hard to get the bug number in the patch when you attach a patch at filing time. :)

> Why not return when the shell is null? Like so:
>     if (!presShell || presShell->IsNeverPainting()) {

We can still ResetWidgetBounds without a presshell fine, that path never looks at the presshell. So if it's okay with you I'll leave this as is.

> Not your fault, but there are two of these assertions with nothing
> significant in-between, please remove the one above.

Okay.


And just now I noticed a silly mistake: I forget to change mPresShell && mPresShell->IsVisible() inside the for loop. I'll fix and re-upload.

Comment 3

[Timothy Nikkel (:tnikkel)]

Attachment: patch v2

Comment 4

[Mats Palmgren (inactive)]

(In reply to Timothy Nikkel (:tn) from comment #2)
> We can still ResetWidgetBounds without a presshell fine, that path never
> looks at the presshell. So if it's okay with you I'll leave this as is.

It's okay to leave as is for now.  (I'm not totally convinced that
state is even valid though.)

> And just now I noticed a silly mistake: I forget to change mPresShell &&
> mPresShell->IsVisible() inside the for loop. I'll fix and re-upload.

Oops, right, I should have caught that.  That's a good indication that
this method with its two principal vm's is error prone, so it's probably
a good idea to split up this method so that the root vm is 'this' in one
method and the view's vm is 'this' in another.  Not necessarily in this
bug though.

Comment 5

[Mats Palmgren (inactive)]

Comment on attachment 8384233 [details] [diff] [review]
patch v2

r=mats

>-      NS_ASSERTION(aView->HasWidget(), "Must have a widget!");

I think I prefer to keep this one and remove the latter one
instead.  This code is inside a "if (widget..." block so
I think the intention is to catch the FlushDelayedResize 
loop doing bad things.  So having it right after that loop
seems more appropriate.  Perhaps indicate that intention in the
assertion message? ("FlushDelayedResize removed our widget!")

Comment 6

[Timothy Nikkel (:tnikkel)]

(In reply to Mats Palmgren (:mats) from comment #4)
> (In reply to Timothy Nikkel (:tn) from comment #2)
> > We can still ResetWidgetBounds without a presshell fine, that path never
> > looks at the presshell. So if it's okay with you I'll leave this as is.
> 
> It's okay to leave as is for now.  (I'm not totally convinced that
> state is even valid though.)

You're right, it may not matter at all, but I was able to string together a plausible path. I think view managers get created before presshells, and the presshell on the viewmanager is set in PresShell::Init. So the presshell may be null on a viewmanager for a short period. nsViewManager::WillPaintWindow is called from widget code, and it calls ProcessPendingUpdates. So there is a slim chance we could hit that case.

Comment 7

[Timothy Nikkel (:tnikkel)]

(In reply to Mats Palmgren (:mats) from comment #5)
> I think I prefer to keep this one and remove the latter one
> instead.  This code is inside a "if (widget..." block so
> I think the intention is to catch the FlushDelayedResize 
> loop doing bad things.  So having it right after that loop
> seems more appropriate.  Perhaps indicate that intention in the
> assertion message? ("FlushDelayedResize removed our widget!")

Done.

Comment 8

[Mats Palmgren (inactive)]

(In reply to Timothy Nikkel (:tn) from comment #6)

OK, it seems plausible, but then...

> ... and it calls ProcessPendingUpdates

which calls ProcessPendingUpdatesForView with aFlushDirtyRegion==true so
then we might crash on the unchecked "presShell->Paint" call?

Comment 9

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/984bd72d320c

Comment 10

[Timothy Nikkel (:tnikkel)]

(In reply to Mats Palmgren (:mats) from comment #8)
> (In reply to Timothy Nikkel (:tn) from comment #6)
> 
> OK, it seems plausible, but then...
> 
> > ... and it calls ProcessPendingUpdates
> 
> which calls ProcessPendingUpdatesForView with aFlushDirtyRegion==true so
> then we might crash on the unchecked "presShell->Paint" call?

Oh, whoops, I intended to guard that, but forgot. I'll do a quick fix up.

Comment 11

[Timothy Nikkel (:tnikkel)]

Attachment: null check presshell

Comment 12

[Mats Palmgren (inactive)]

Comment on attachment 8384241 [details] [diff] [review]
null check presshell

Please move the "if (presShell) {" up a few lines so it also
includes the first MOZ_DUMP_PAINTING block.

Comment 13

[Timothy Nikkel (:tnikkel)]

Moved the if up and landed
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b678e143116
Thanks.

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/984bd72d320c
https://hg.mozilla.org/mozilla-central/rev/5b678e143116

Comment 15

[Mats Palmgren (inactive)]

Comment on attachment 8384241 [details] [diff] [review]
null check presshell

[Approval Request Comment]
Bug caused by (feature/regressing bug #): - 
User impact if declined: possible null-pointer crash?
Testing completed (on m-c, etc.): on m-c since 2014-03-03
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

The request is for both patches.  I want this on Aurora so that my patch
in bug 946658 applies.

Comment 16

[Mats Palmgren (inactive)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3af224d9f248
https://hg.mozilla.org/releases/mozilla-aurora/rev/ab90efe51b6e

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/5b5b120f02a1
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/08183adc41f6