191246 - [FIX]Crash [@ nsObjectFrame::GetDesiredSize] when object is fixed pos

Status: RESOLVED FIXED

(Core :: Layout: Positioned, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

STEPS TO REPRODUCE: Click the link in the "url" field

ACTUAL RESULTS: crash

EXPECTED RESULT: act like a browser instead of wimping out

PROBLEM: Bug 131065 introduced the following code in nsObjectFrame::GetDesiredSize:

904     else {// unconstrained percent case
905       nsCOMPtr<nsIAtom>  fType;
906       const nsHTMLReflowState* cbrs = aReflowState.parentReflowState;
907       nsIFrame* f = cbrs->parentReflowState->frame;
908       f->GetFrameType(getter_AddRefs(fType));

This code has one little problem -- it's missing some major null-checking,
starting with the fact that for fixed-positioned frames there _is_ no parent
reflow state (this coule be construed as a bug in the viewport frame, but read
on).  If there were, you're getting the _grandparent_ too and not checking it.

Comment 1

[Boris Zbarsky [:bzbarsky]]

jerry, please let me know asap if you won't have time to fix this, ok?  Thank you.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Oh, and the code that does

936         else NS_ASSERTION(containingBlockRS, "no containing block");

is 1)  Silly (just use NS_ERROR) and 2) Wrong (since that's a perfectly valid
situation to be in).

Finally, I can't shake the feeling that when you do

const nsHTMLReflowState* containingBlockRS = aReflowState.parentReflowState;

you mean mCBReflowState, not parentReflowState (and that should save you all
that manual state-chain walking too, since it's all rolled into
mCBReflowState)... but I can't figure out what "problem" this code is trying to
solve (some documentation as to _why_ you're doing as opposed to _what_ you're
doing would have been mighty nice here)...

Comment 3

[Jerry]

Attachment: testcase

Comment 4

[Boris Zbarsky [:bzbarsky]]

I am tempted to back out bug 131065 and reopen it for a real fix, since the
patch that landed just seems incredibly bogus to me.  Anyone have any better
suggestions?

Comment 5

[Asa Dotzler [:asa]]

This crasher is pretty low in the talkback data. I don't think we should hold
1.3b for this. That's not to say we wouldn't take a fix if we had one.

Comment 6

[Jerry]

assign it to me.

Comment 7

[Jerry]

bz, add a null check maybe easy,
but I dont know what is the size of the object if its position is fixed,
and with percent height.

Comment 8

[Hixie (not reading bugmail)]

Shouldn't the layout engine be dealing with all this on a generic basis? Why on
earth does the <object> element need to know about any of this? Calculating
one's width from a percentage is a hugely complicated problem (think inlines,
run-in, etc) and you can't just take your parent or grandparent to work it out.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Ian is right.  The mComputedHeight in the reflow state should not be
unconstrained, imo, if the height is a percentage value for a fixed-pos element.
 It should be that percentage of the viewport, which is its containing block.

Which brings me to the question... why is this code ignoring the
mComputedHeight/mComputedWidth in the reflow state???  Is there a reason the
reflow state is not useful in this case?  nsImageFrame seems to do just fine
with using it....  And I suspect if you just used them when they are not
unconstrained things would work just fine.

Let's fix this correctly, ok?  Just a null-check is _not_ the right fix.

Again, what is this code trying to do?  Can someone explain that in words?  What
is the problem we are trying to solve, at least?

Comment 10

[Jerry]

Attachment: patch for it

Let's explain bug 131065 's history 
  It is a regresstion caused by 106602
  Here is some code from its patch.
   ...
    if (NS_UNCONSTRAINEDSIZE != aReflowState.availableHeight)
      aMetrics.height = NSToCoordRound (factor * aReflowState.availableHeight);

    else // unconstrained percent case
      aMetrics.height = (NS_UNCONSTRAINEDSIZE == aReflowState.mComputedHeight)
? 0 : aReflowState.mComputedHeight;
  }
 
it works well in some case.

but it ignor some other case, 
you can see http://bugzilla.mozilla.org/show_bug.cgi?id=131065#c75

so bug 131065 exists.

after reading bz's comments, I notice that I make a mistake,
I can use aReflowState.mComputedHeight, if it is not zero.

Comment 11

[Boris Zbarsky [:bzbarsky]]

mComputedHeight should already have the factor and all that stuff in it.  You
shouldn't have to be doing any of that "check my own style" stuff at all....

That, and as I said you want to walk the containing block reflow state chain,
not the parentReflowState chain, no?

Comment 12

[Boris Zbarsky [:bzbarsky]]

See bug 131065 comment 91 and bug 131065 comment 92.

Basically, the patch in bug 131065 completely broke sizing of percentage-sized
<object> elements, even in standards mode.

The cause, I suspect, is once again the lack of use of the mComputedHeight/Width.

Jerry, can you possibly get a patch for this working in time for 1.3?  If not,
do we want to back out bug 131065?  It seems to me that the cure is worse than
the disease, unless I'm missing something major here.

Comment 13

[Asa Dotzler [:asa]]

We're not seeing this in topcrash data. Does this break real-world sites. If
this is a regression for september then this made it into at least one major
release and still hasn't generated lots of dupes or topcrash data. 
I thikn we'd take a safe fix for a crash but we're not going to hold the release
for this. 

Comment 14

[Boris Zbarsky [:bzbarsky]]

> Does this break real-world sites.

Yes.  This crash was initially noted on a real-world site (forget the fact that
it breaks layout on real-world sites).

Could someone involved in the initial fix please give me an evaluation of how
many sites would break if we backed out bug 131065?

Comment 15

[Boris Zbarsky [:bzbarsky]]

Jerry says he has no plans to work on this.... I'll try to get to it sometime....

Comment 16

[Boris Zbarsky [:bzbarsky]]

jkeiser, this is the bug you were interested in....

I've put some testcases in
http://web.mit.edu/bzbarsky/www/testcases/object-layout/ -- the actual testcases
are in the four subdirs; the toplevel dir just has some template files and a
script to generate the testcases.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

This fixes this crash, the sizing issues raised in bug 131065, the
sizing/placement issues in bug 196280.	It doesn't seem to break anything too
badly, except the case of %-height plugins in standards mode in auto-height
parents (they get 0 height).  Which I'd be happy to fix if someone can come up
with a reasonable way to get the intrinsic size of a plugin (hint, hint; can we
add this to the plugin API??)

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 119538 [details] [diff] [review]
Proposed patch

Peter, David, would you review?

David, this is largely your patch from bug 131065 (I still don't understand why
it did not get used!), but I made the case when we have a child frame a little
more predictable (should reflow like the <object> frame isn't there, more or
less).

Comment 19

[Boris Zbarsky [:bzbarsky]]

Oh, this fixes some other sizing issues too (eg look at the iframe/image
testcases in a current nightly on the various table testcases....)

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Why can't %-height plugins in standards mode be treated like auto-height plugins?

Comment 21

[Boris Zbarsky [:bzbarsky]]

They are.  Auto-height plugins get 0 height (both before my patch and after it).
 See
http://web.mit.edu/bzbarsky/www/testcases/object-layout/object-layout-flash-with-width/simpleUnconstrained.html
for the testcase.

I suppose we could just make up a random height for auto-height plugins... Or we
could fix the plugin API to allow asking the plugin for its preferred size.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 119538 [details] [diff] [review]
Proposed patch

I want to make a few tweaks here....

Comment 23

[Boris Zbarsky [:bzbarsky]]

Attachment: Version 1.1

Better comments, more cleanup, better HandleChild code...

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 119582 [details] [diff] [review]
Version 1.1

Note, the nsIWidget include needs to move from the cpp to the header....

Comment 25

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

Looks good basically, but one question: if we can't get an intrinsic size,
wouldn't it make more sense to use the EMBED_DEF default size instead of "0"?
For Web authors, at least, the former lets you know that the plugin is there
even if it has the wrong size ... somewhat less confusing than just failing to
appear.

Comment 26

[Boris Zbarsky [:bzbarsky]]

I considered doing that...  If Peter is OK with it, I'll do it, sure. The only
danger is things sizing "too big", and since it seems that IE gives some sort of
default size to things, I'm not sure how much of a danger that is....

Comment 27

[Peter Lubczynski]

I'm pulling a tree now and I'll need to run through a few sites to test the patch.

See bug 70976 about intrinsic sizing for plugins. The old XPCOM Plugin API
provided a method called SetWindowSize that was never implemented in
nsPluginInstancePeer.cpp.

Also, would it be possible for the default size to be specified in CSS?

Comment 28

[Boris Zbarsky [:bzbarsky]]

We can specify the default size in CSS, but the author can always override
it to "auto" in the author style sheet.  So we need the fallback in
the C++ anyway....

Comment 29

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

I don't think we can specify a default size in CSS just for plugins, right? It
would apply to all OBJECTs including images and IFRAMEs. So we probably don't
want to do that.

Comment 30

[Boris Zbarsky [:bzbarsky]]

Yeah, that too.

Comment 31

[Peter Lubczynski]

Comment on attachment 119582 [details] [diff] [review]
Version 1.1

r=peterl

Comment 32

[Peter Lubczynski]

Comment on attachment 119582 [details] [diff] [review]
Version 1.1

oh...I think nsObjectFrame.h needs a #include "nsIWidget.h" now that it's a
COMPtr, seems that accessible depends on it.

Comment 33

[Boris Zbarsky [:bzbarsky]]

Yes, see comment 24.  ;)

Comment 34

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 119582 [details] [diff] [review]
Version 1.1

roc, I think I'd like to go with this for now and find someone with IE to test
before I try to do a default size for <object>....

That said, Peter and I have been talking about trying to get real intrinsic
sizing working.

Anyway, let me know please if you really want default nonzero size as part of
this patch....

Comment 35

[Boris Zbarsky [:bzbarsky]]

Checked in.

Comment 36

[Joshua Xia]

This fixing lead to regression, see:
http://www.mozilla.org/quality/browser/front-end/testcases/oji/test7.html

the x% height don't work.

Comment 37

[Boris Zbarsky [:bzbarsky]]

That was done on purpose.  Per spec, those percent heights should do absolutely
nothing in this case.  If you feel that we must absolutely have a quirk for this
in quirks mode (we already have some quirks for the common case of 100% height
<applet>/<object> in a table cell), please file a separate bug requesting such a
quirk.  But I'd need to see sites that actually break due to lack of said quirk...

In general, please file new bugs for regressions (and comment in the bug that
caused the regression, but do not reopen it).