336147 - Changing innerHTML for <input type="button"> causes miss-rendering

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Ryan Jones-Ward [:sciguyryan]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060501 Minefield/3.0a1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060501 Minefield/3.0a1

When the design mode for the document is set explicitly to be off then clicking on  the button in the test case URL above accesses the innerHTML property of a HTML element that should not have an innerHTML property.

Setting design mode too on fixes this problem:

http://standards.spiralmindsinc.com/misc/designmode-testcases/innerhtml-02-designmode-on.html

Has been confirned on:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060501 Minefield/3.0a1
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20060430
BonEcho/2.0a1
Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.0.2)
Gecko/20060308 Firefox/1.5.0.2

Reproducible: Always

Steps to Reproduce:
1. Go the the URL above.
2. Click the button.

Actual Results:  
Somehow the innerHTML property is accessed for an element that should not have it and changed breaking the button.

Expected Results:  
Nothing, innerHTML is invalid for this element so should have returned a Javascript error.

Comment 1

[Martijn Wargers (dead)]

Setting designMode to on disables javascript, so that's why it 'fixes' the problem.
DesignMode is not related to this.
innerHTML should work just fine for this replaced element, but it's probably more logical when the contents wouldn't show up.
The text input you get to see also, is because of bug 291047.

Reporter, could you attach the testcase to the bug?

Comment 2

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Testcase attatched as per Martijn Warger' request.

Comment 3

[Ryan Jones-Ward [:sciguyryan]]

(In reply to comment #2)
> Created an attachment (id=220412) [edit]
> Testcase attatched as per Martijn Warger' request.
> 

Done. Anything else needed?

Comment 4

[Martijn Wargers (dead)]

(In reply to comment #3)
> (In reply to comment #2)
> > Created an attachment (id=220412) [edit]
> > Testcase attatched as per Martijn Warger' request.
> > 
> 
> Done. Anything else needed?

A patch? ;)

Comment 5

[Ryan Jones-Ward [:sciguyryan]]

(In reply to comment #4)
> A patch? ;)
> 

I'll have to start learning C++ then ;)

Comment 6

[Boris Zbarsky [:bzbarsky]]

So the problem is that our IsLeaf() checks are performed on the "parent" frame in  nsCSSFrameConstructor::ContentAppended/ContentInserted.  But that "parent" is the content insertion frame for the primary frame of our parent content.  And of course the content insertion frame of a button is an Area, which is not a leaf.

We could try to hack the frame constructor to look at both frames, I guess, but I think a possibly better approach is to change nsFrame::IsLeaf from |return PR_FALSE| to |return mParent && mParent->GetContentInsertionFrame() == this && mParent->IsLeaf()|.  At least assuming that our content insertion frames are always direct kids of the parent; are they?

Comment 7

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

I wouldn't bet on it. For example an overflow:auto column-count:2 block would have a nsGfxScrollFrame wrapping an nsColumnSetFrame wrapping one or more block frames.

I think the IsLeaf check should simply be performed on the primary frame for the content.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Ok.  So we basically need to change all the callers of IsLeaf() to do that (probably in addition to looking at the content insertion frame).

Ryan, you feeling up to doing that?

Comment 9

[Ryan Jones-Ward [:sciguyryan]]

I'll take a shot :)

Comment 10

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

I don't think we should look at the content insertion frame, just at the primary.

Comment 11

[Boris Zbarsky [:bzbarsky]]

If we don't look at the insertion frame and it _is_ by some chance a leaf (like not a container frame), we'll crash in exploitable ways after trying to append frames to it.  So we should either be looking at it, or asserting in all GetContentInsertionFrame implementations that the return value is either |this| or a non-leaf.

Comment 12

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

I guess you're right and we should check both, although that seems incredibly lame.

Actually it seems strange that <input type="button"> implements GetContentInsertionFrame() at all, since we shouldn't be inserting content under it, except for the anonymous content. And I believe we don't use GetContentInsertionFrame when creating anonymous content, we always plug it in under the frame that created it. So I think nsGfxButtonControlFrame could just override GetContentInsertionFrame and return "this"...

Comment 13

[Boris Zbarsky [:bzbarsky]]

Hmm.  That's a good point.  I guess it really doesn't make sense for leaves to implement GetContentInsertionFrame().

How about we do that, and have GetFrameFor() assert that either the insertion frame is the primary frame, or the primary is not a leaf?

Comment 14

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

sure

Comment 15

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v1

Patch v1

* Overrides the |GetContentInsertionFrame| method in |nsGfxButtonControlFrame|. Asserts in |nsCSSFrameConstructor::GetFrameFor| if the insertion frame is the primary frame or the primary frame isn't a leaf.

Comment 16

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

+  return NS_STATIC_CAST(nsIFrame*, this);

You shouldn't need the cast here. Otherwise fine.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Nix the whitespace changes?

Comment 18

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v1.1

Patch v1.1

* Reverted the space changes (thanks bz) and no longer use |NS_STATIC_CAST(nsIFrame*, this)| in favour of just returning |this| directly.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 255327 [details] [diff] [review]
Patch v1.1

I think roc can happily r+sr this.  ;)

Comment 20

[Martijn Wargers (dead)]

Checking in base/nsCSSFrameConstructor.cpp;
/cvsroot/mozilla/layout/base/nsCSSFrameConstructor.cpp,v  <--  nsCSSFrameConstru
ctor.cpp
new revision: 1.1313; previous revision: 1.1312
done
Checking in forms/nsGfxButtonControlFrame.cpp;
/cvsroot/mozilla/layout/forms/nsGfxButtonControlFrame.cpp,v  <--  nsGfxButtonCon
trolFrame.cpp
new revision: 1.139; previous revision: 1.138
done
Checking in forms/nsGfxButtonControlFrame.h;
/cvsroot/mozilla/layout/forms/nsGfxButtonControlFrame.h,v  <--  nsGfxButtonContr
olFrame.h
new revision: 1.49; previous revision: 1.48
done

Checked into trunk.

Thanks for the patch, Ryan!

Comment 21

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Reftest v1

First shot at a reftest for this.

Comment 22

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Reftest v1

Get rid of the new line warning.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 257529 [details] [diff] [review]
Reftest v1

Looks good.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Checked in the test.