54850 - crash when plugin has an inline parent

Status: RESOLVED WORKSFORME

(Core Graveyard :: Plug-ins, defect, P1)

Description

[Doron Rosenberg (IBM)]

This site uses flash, and loads totally different than in ns4. First, most of
the flash is gone, and 2nd, the Jobs logo keeps moving to the right. There is
also a crash on first load in mozilla 20000093008.

I think all these issue are based on the fact that the flash is not loading.
Sending to Embedding.

Comment 1

[John Morrison]

Actually, Plug-ins is the place for Flash, etc. (Embedding APIs is for the
case of embedding the layout engine into some other application).

By the way, on win2k, Nav4.74 doesn't load this page correctly, and IE5 offers
to install Flash5.0 Aug2000, and when I decline, it doesn't load much of
anything either.

Comment 2

[shrirang khanzode]

Actually, I can see this working fine on 4.72. I see what Doron is saying. The  
"Jobs" button just keeps on going to the right ... :-)..funny !  Isee this in 
093008 m18 buils on windows.

Comment 3

[Doron Rosenberg (IBM)]

jrgm - sorry, stupid me ;(

setting to windows, will try to get this tested on linux/mac.

Comment 4

[Jonas Nagel]

there is - repeatetly proven - a crash on windows (win32), I've just sent in a 
stack trace with the working Talkback bits from today, adding keyword crash, and 
changing to major

Comment 5

[av (gone)]

It is not always consistent, but the problem is (I think) that a negative value 
is passed to nsViewManager::ResizeView as width. Actually, no plugin code 
involved here, think this is strictly layout. Adding buster. Steve if I remember  
correctly you fixed a neg-width problem once. I am attaching the stack trace 
when the assertion occurs.

Comment 6

[av (gone)]

Attachment: Call stack on assertion

Comment 7

[cata]

I am able to reproduce this on my Win2k with the a 10/19 pull 
Netscape_20000922_BRANCH.
Looking at the HTML code, it seems that the assertion is caused by the 
interaction between the two tags:
<img width="119" height="54">
<OBJECT WIDTH=100% HEIGHT=100%>

Deleting any of them elliminates the assertion. Some bug in the layout/resize 
code?

Comment 8

[ekrock's old account (dead)]

Nominating for rtm consideration as a flash crasher. Reassigning to buster for
investigation because the cause is believed to lie in layout. Steve, could you
please investigate?

Comment 9

[buster]

looking into this now.

Comment 10

[timeless]

flash has a keyword

Comment 11

[buster]

I have a stability fix for this that is a simple check to make sure we never let
the width or height of a frame go negative.  But that doesn't make the page lay
out correctly, or get to the heart of the problem.  So I'm going to spend the
rest of the day trying to fix this correctly in nsObjectFrame.cpp, which is
misbehaving badly.

Comment 12

[buster]

I'm about to attach fixes in two files.  They are totally independent, and I
could check in either or both for rtm.

1)The important fix is in nsObjectFrame.cpp.  It applies a fix done by me long
ago for object height measurement to width measurement.  It is extremely safe
and clearly the right thing to do.
2) A second fix in nsLineLayout.cpp.  It verifies that child frames always
return a non-negative width and height.  As a purely defensive move, it sets
negative values to 0 before proceeding.  This protects the block/inline code
from misbehaving frames.  This is as close to 0 risk as a fix can be.  Negative
values are highly illegal, and guaranteed to crash.  We can do nothing but good
for release code by putting in this check (with assertions to alert developers
to misbehaving child frames.)

av, peterl:  please review
waterson:  please super-review

Comment 13

[buster]

Attachment: the fix to nsObjectFrame.cpp

Comment 14

[buster]

Attachment: the fix to nsLineLayout.cpp

Comment 15

[buster]

making summary more clear
this problem will cause a crash whenever any object frame (ie, plugin) is
contained within an inline frame and that inline frame is horizontally offset
from it's containing blockby more than half the width of the object frame.
Clear as mud?

The point is, any plugin can trigger the crash simply by virtue of where it is
within the geometry of the page.   So some pages will crash on page load.
Others might only crash at certain window sizes, or as the window is resized.
Other pages might crash as a result of javascript manipulating the document
content or geometry.  Still others might crash based on when and how images load
on that page.  Etc.  It's bad, and it needs to be fixed.

Comment 16

[Peter Lubczynski]

both patches look good, r=peterl

Comment 17

[Chris Waterson]

sr=waterson

I agree that the math is wakcy. Maybe we can get ianh or jrgm to tinker around
with object borders, padding, etc. where left!=right and top!=bottom, and file
new bugs if necessary.

Comment 18

[av (gone)]

Looks like GetWidth method did not get the fix which went into GetHeight some 
time ago. r=av.

Comment 19

[Jim Roskind]

Moving this bug into candidate limbo.
Please check it into the trunk asap, and report any regressions.
Anticipate that this will probably be changed to rtm double plus on Monday, as
we think we currently have a fall-back RTM candidate in hand.

Comment 20

[buster]

fix checked into trunk

Comment 21

[Phil Peterson]

PDT marking [rtm++]. This bug is now out of limbo and approved for checkin to
the branch. Please check in ASAP.

Comment 22

[Doron Rosenberg (IBM)]

this is fixed win98 2000103004 mozilla trunk.  

Comment 23

[shrirang khanzode]

Is this fixed in the branch ?  

Comment 24

[buster]

yes, this fix was checked in on both the NS branch and on the trunk.

Comment 25

[Kathleen :Brade]

I just crashed with my Mac branch build from around 1am PST (trying to load the
url set above).  I don't think this bug is fixed.

Comment 26

[Doron Rosenberg (IBM)]

brade: did the flash load or did the jobs icon move to the right all the time
when you crashed?

Comment 27

[gogden]

using build 2000103108 os9 g4 this site loaded - no crash - button did not migrate

Comment 28

[Kathleen :Brade]

I tried again and hung; waiting for new bits to test with

Comment 29

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Using commercial release build 2000-10-31-14 (branch) on NT I do not see crash.

Comment 30

[shrirang khanzode]

Looks good on windows branch build 2000-10-31-14-MN6. No crash and page loads 
fine. Plugin used: flash 5.0. Adding keyword: vtrunk

Comment 31

[Kathleen :Brade]

The crash I saw yesterday was probably unrelated to this crash; with a Mac build
(2000103114) I can't reproduce the crash I saw (it was probably the blocker
crash bug).

Comment 32

[Doron Rosenberg (IBM)]

verified on trunk win98, linux and mac (thanks to asa).

Comment 33

[Jonas Nagel]

I still see this (crash) happening today on the branch build (20001108), 
Win2000. :/ Maybe you should check that again...

Comment 34

[Jonas Nagel]

OK, I sent in two talkback reports - it now also happened on the mtrunk build to 
me. Talkback ID of crash on mtrunk build: TB20707879G
I lost the ID of the report on the branch build.

Comment 35

[shrirang khanzode]

works on 11/08 branch build on windows.

Comment 36

[[e-mail]]

I couldn't get the page to crash after several reloadings and closing/reopening
page.
AMD K6-2 350MHz, 64M
WinME
Mozilla Trunk Build 2000110804
User-Agent: Mozilla/5.0 (Windows; U; Win 9x 4.90; en-US; m18) Gecko/20001108

Comment 37

[buster]

This works fine for me on WinNT, 20001107.  Could somebody in QA test on
Win2000? I'm concerned that someone reports that it still crashes.  Could
somebody attach the talkback report (or at least the url) as well?

Comment 38

[shrirang khanzode]

cannot reproduce crash on windows 2000 (20001107) build. Working fine.

Comment 39

[Peter Lubczynski]

WFM using Windows 2000 using a fresh pull of the trunk this morning and works
with the 11/9 branch build.

However, looking at the talkback report, this could still be happening.
Jonas, is there any more information you can give us to reproduce this?

Comment 40

[Jonas Nagel]

Hmm, today that URL crashed my Mozilla on Win32 (Build 2001020604) again. But 
only the first time I visited that URL (http://www.x-tra.ch). Can others please 
doublecheck?

Comment 41

[shrirang khanzode]

yep, crashes with today's trunk on windows 20010206. Reopening.(stack trace 
coming up..)

Comment 42

[shrirang khanzode]

Attachment: stack trace

Comment 43

[Chris Waterson]

This is working fine for me. I do have flash installed (which is what this site
appears to use). What about the people that are reporting crashes?

Comment 44

[Jonas Nagel]

Yes Flash is installed. The second time I visited that page it didn't crash and 
the Flash navigation displayed properly. *shrug*

Comment 45

[Chris Waterson]

taking

Comment 46

[Chris Waterson]

Well, I cannot for the life of me reproduce this bug. If anyone can come up with
a reliable test case, attach it and re-open.