Last Comment Bug 282105 - [FIX]BoxObject's InvalidatePresentationStuff needs ability to tell document to remove it from its boxobject table
: [FIX]BoxObject's InvalidatePresentationStuff needs ability to tell document t...
Status: RESOLVED FIXED
[sg:critical] [no l10n impact][rft-dl]
: fixed-aviary1.0.8, fixed1.7.13, fixed1.8.1, verified1.8.0.2
Product: Core
Classification: Components
Component: XUL (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: mozilla1.9alpha1
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
: 282104 (view as bug list)
Depends on: 282103 326834
Blocks: 274712 322513 325045 457353
  Show dependency treegraph
 
Reported: 2005-02-13 00:12 PST by Ben Goodger (use ben at mozilla dot org for email)
Modified: 2008-09-26 17:59 PDT (History)
18 users (show)
timr: blocking1.7.13+
timr: blocking‑aviary1.0.8+
benjamin: blocking1.8b3-
bugs: blocking1.8b5-
bugs: blocking‑aviary1.5-
timr: blocking1.8.0.2+
bob: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (9.31 KB, patch)
2005-02-13 00:12 PST, Ben Goodger (use ben at mozilla dot org for email)
bzbarsky: review-
Details | Diff | Review
This should do the trick. Testing now.... (26.25 KB, patch)
2006-01-29 17:19 PST, Boris Zbarsky [:bz]
neil: review+
bugs: superreview+
Details | Diff | Review
Same for 1.8 branch (25.97 KB, patch)
2006-01-29 20:49 PST, Boris Zbarsky [:bz]
neil: review+
bugs: superreview+
neil: approval‑branch‑1.8.1+
dveditz: approval1.8.0.2+
Details | Diff | Review
Same for 1.7/aviary branches (26.22 KB, patch)
2006-01-29 20:51 PST, Boris Zbarsky [:bz]
neil: review+
bugs: superreview+
dveditz: approval‑aviary1.0.8+
dveditz: approval1.7.13+
Details | Diff | Review

Description Ben Goodger (use ben at mozilla dot org for email) 2005-02-13 00:12:09 PST
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) 2005-02-13 00:12:38 PST
Created attachment 174197 [details] [diff] [review]
patch
Comment 2 :Gavin Sharp [email: gavin@gavinsharp.com] 2005-02-13 00:13:35 PST
*** Bug 282104 has been marked as a duplicate of this bug. ***
Comment 3 Boris Zbarsky [:bz] 2005-02-14 11:15:49 PST
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) 2005-07-18 16:28:19 PDT
No longer blocking anything. 
Comment 5 Boris Zbarsky [:bz] 2005-07-18 16:46:24 PDT
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 [:bz] 2006-01-09 18:41:18 PST
This is causing toolkit users problems, apparently.  See bug 322513.  I do think we should fix this...
Comment 7 Boris Zbarsky [:bz] 2006-01-29 16:01:28 PST
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 [:bz] 2006-01-29 17:19:27 PST
Created attachment 210093 [details] [diff] [review]
This should do the trick.  Testing now....
Comment 9 Boris Zbarsky [:bz] 2006-01-29 20:49:28 PST
Created attachment 210109 [details] [diff] [review]
Same for 1.8 branch
Comment 10 Boris Zbarsky [:bz] 2006-01-29 20:51:07 PST
Created attachment 210110 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2006-01-30 06:15:51 PST
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 neil@parkwaycc.co.uk 2006-01-30 06:16:28 PST
Comment on attachment 210109 [details] [diff] [review]
Same for 1.8 branch

Some previous comments may be relevant.
Comment 13 Boris Zbarsky [:bz] 2006-01-30 07:43:59 PST
> 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 [:bz] 2006-01-30 07:45:06 PST
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] 2006-01-30 12:16:51 PST
BZ please land on trunk!
Comment 16 Daniel Veditz [:dveditz] 2006-01-30 12:17:53 PST
...when you have the reviews you need.
Comment 17 Ben Goodger (use ben at mozilla dot org for email) 2006-01-30 12:23:20 PST
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) 2006-01-30 12:26:44 PST
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) 2006-01-30 12:28:04 PST
Comment on attachment 210093 [details] [diff] [review]
This should do the trick.  Testing now....

sr=ben@mozilla.org
Comment 20 Ben Goodger (use ben at mozilla dot org for email) 2006-01-30 12:28:36 PST
Comment on attachment 210109 [details] [diff] [review]
Same for 1.8 branch

sr=ben@mozilla.org
Comment 21 Ben Goodger (use ben at mozilla dot org for email) 2006-01-30 12:29:07 PST
Comment on attachment 210110 [details] [diff] [review]
Same for 1.7/aviary branches

sr=ben@mozilla.org
Comment 22 Boris Zbarsky [:bz] 2006-01-30 13:30:08 PST
Fixed on trunk.
Comment 23 David G King 2006-01-30 14:45:43 PST
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 [:bz] 2006-01-30 16:11:38 PST
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 [:bz] 2006-01-30 19:46:18 PST
Fixed on 1.8.1 branch.
Comment 26 Daniel Veditz [:dveditz] 2006-02-02 14:53:46 PST
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] 2006-02-02 14:54:01 PST
Comment on attachment 210110 [details] [diff] [review]
Same for 1.7/aviary branches

a=dveditz for drivers, ditto
Comment 28 Boris Zbarsky [:bz] 2006-02-02 17:53:38 PST
Fixed on MOZILLA_1_8_0_BRANCH, MOZILLA_1_7_BRANCH, AVIARY_1_0_1_20050124_BRANCH.
Comment 29 Marcia Knous [:marcia - use ni] 2006-02-16 14:19:52 PST
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 - use ni] 2006-02-16 14:27:32 PST
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 [:bz] 2006-02-16 14:41:02 PST
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] 2006-02-24 17:27:45 PST
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)

Note You need to log in before you can comment on or make changes to this bug.