282105 - [FIX]BoxObject's InvalidatePresentationStuff needs ability to tell document to remove it from its boxobject table

Status: RESOLVED FIXED

(Core :: XUL, defect, P1)

Description

[Ben Goodger (use ben at mozilla dot org for email)]

We can get into situations where an element's box object becomes useless because
of some sequence of events like this: frames are destroyed (e.g. in response to
opacity changes (?)), InvalidatePresentationStuff is called on the box object in
question, but the document still holds it in its boxobject table, so that when
script attempts to subsequently access boxobject methods, the busted boxobject
with null mPresShell etc is used, and none of the methods work. 

This was noted in earlier testing versions of my new prefwindow where I had a
listbox and was attempting to access properties on it after the pane was faded in. 

The solution appears to be that in some cases when InvalidatePresentationStuff
is called, we want to tell the document to take it out of the boxobject table,
which is what this patch does. See also the patch in 282103, which updates the
call sites in nsXULDocument.

Comment 1

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: patch

Comment 2

[:Gavin Sharp [email: [e-mail]]]

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

Comment 3

[Boris Zbarsky [:bzbarsky]]

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

So... the only caller with PR_TRUE will be nsListBoxBodyFrame::Destroy, right? 
In that case, may I ask why this is nuking the box object in the first place?

It seems to me like the listbox is just abusing the
InvalidatePresentationStuff() api here; while it should be setting the
listboxbody property to null when the listbox frame is destroyed, it should NOT
be nulling out the presshell.  It seems that it wants a separate method it can
call on the nsListBoxObject that will do that.

Comment 4

[Ben Goodger (use ben at mozilla dot org for email)]

No longer blocking anything. 

Comment 5

[Boris Zbarsky [:bzbarsky]]

Might I ask why not?  I can see that it might not be blocking Firefox 1.1, but
why isn't it blocking toolkit/gecko 1.8?

Comment 6

[Boris Zbarsky [:bzbarsky]]

This is causing toolkit users problems, apparently.  See bug 322513.  I do think we should fix this...

Comment 7

[Boris Zbarsky [:bzbarsky]]

OK, this also causes a crash that's most likely an exploitable security hole (bug 325045).  Further, in debugging that I figured out that the way this is set up means that the 1.7 branch is also exploitable in the same way (since content can call setPropertyAsSupports).

I guess I'll take this and see what I can do here.  :(  Does rather make me wish someone had answered my question from comment 5....

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: This should do the trick. Testing now....

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Same for 1.8 branch

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Same for 1.7/aviary branches

So what these patches do is make sure that content JS

1)  Can't get hold of the list box body frame via XPConnect.
2)  Can't mess with our boxobject's internal frame pointers.

Both are accomplished by having some nonscriptable private interfaces.

Comment 11

[[e-mail]]

Comment on attachment 210093 [details] [diff] [review]
This should do the trick.  Testing now....

>+  // listboxBody is always null.  It's only here to avoid changing the
>+  // interface.
Surely we can just this completely on trunk?

>+                 "Calling SetPropertyAsSupports on a frame.  Prepare to crash "
>+                 "and be exploited any time some random website decides to "
>+                 "exploit you");
Repetition of /\bexploit/ :-P I think "and be exploited" is obvious given the following text.

>-  SetPropertyAsSupports(NS_LITERAL_STRING("listboxbody").get(), nsnull);
>+  ClearCachedListBoxBody();
Using a virtual call to set a member variable?

Comment 12

[[e-mail]]

Comment on attachment 210109 [details] [diff] [review]
Same for 1.8 branch

Some previous comments may be relevant.

Comment 13

[Boris Zbarsky [:bzbarsky]]

> Repetition of /\bexploit/ :-P I think "and be exploited" is obvious given the
> following text.

The following text doesn't make sense without the last part, unless I'm missing something.  Care to suggest the wording you meant?

> Using a virtual call to set a member variable?

Yep.  It's cleaner in this case, and this is not perf-intensive code.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Oh, missed one comment:

> Surely we can just this completely on trunk?

I'd love to do that as a separate patch, but I wanted to minimize merging as much as possible.  As it was, it took me a good long while to get this stuff merged...

Comment 15

[Tim Riley [:timr]]

BZ please land on trunk!

Comment 16

[Daniel Veditz [:dveditz]]

...when you have the reviews you need.

Comment 17

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 210093 [details] [diff] [review]
This should do the trick.  Testing now....

> NS_IMETHODIMP
> nsListBoxObject::GetListboxBody(nsIListBoxObject * *aListboxBody)
> {
>-  *aListboxBody = GetListBoxBody();
>-  NS_IF_ADDREF(*aListboxBody);
>+  *aListboxBody = nsnull;
>   return NS_OK;
> }

Why does this return null now?

Comment 18

[Ben Goodger (use ben at mozilla dot org for email)]

One of these days I'll learn to read scrollback. Ignore my previous question. 

Comment 19

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 210093 [details] [diff] [review]
This should do the trick.  Testing now....

sr=[e-mail]

Comment 20

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 210109 [details] [diff] [review]
Same for 1.8 branch

sr=[e-mail]

Comment 21

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 210110 [details] [diff] [review]
Same for 1.7/aviary branches

sr=[e-mail]

Comment 22

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk.

Comment 23

[David G King]

This checkin caused the following on my Win32 cygwin MingW Thunderbird build :-

e:/mozilla/source/mozilla/layout/xul/base/src/nsListBoxObject.cpp:87: error: ext
ra `;'

I think it actually means line 70 (as that is what my Linux build complains about).

Comment 24

[Boris Zbarsky [:bzbarsky]]

jst fixed the build bustage and I'll make sure to fix it in the branch patches before landing.

For future reference, please just comment; don't reopen.  That doesn't send _nearly_ as much bugspam.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Fixed on 1.8.1 branch.

Comment 26

[Daniel Veditz [:dveditz]]

Comment on attachment 210109 [details] [diff] [review]
Same for 1.8 branch

a=dveditz for drivers. Please land soon so we can shake this out.

Comment 27

[Daniel Veditz [:dveditz]]

Comment on attachment 210110 [details] [diff] [review]
Same for 1.7/aviary branches

a=dveditz for drivers, ditto

Comment 28

[Boris Zbarsky [:bzbarsky]]

Fixed on MOZILLA_1_8_0_BRANCH, MOZILLA_1_7_BRANCH, AVIARY_1_0_1_20050124_BRANCH.

Comment 29

[Marcia Knous [:marcia]]

Boris: If you could provide a quick way for QA to verify the fix in the bug for 1.0.8/1.7.13, that would be very helpful.

Comment 30

[Marcia Knous [:marcia]]

Boris: Ignore my last comment, please. We won't get to verifying some of the non-critical bugs for this release.

(In reply to comment #29)
> Boris: If you could provide a quick way for QA to verify the fix in the bug for
> 1.0.8/1.7.13, that would be very helpful.
> 

Comment 31

[Boris Zbarsky [:bzbarsky]]

Marcia, the bugs this bug blocks have testcases that should be verified (that's bug 322513 and bug 325045).  Note that one of them is critical but just didn't have the fixed-* keywords set, since they were set on this bug (which covered the main problem).  I'll try to set fixed-* keywords on all bugs a checkin fixes in the future, not just the bug where the work is happening.

Comment 32

[Jay Patel [:jay]]

v.fixed on 1.8.0 branch with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.1) Gecko/20060224 Firefox/1.5.0.1, based on verifications of bug 322513 and bug 325045 (pre bz in comment #31)