237760 - Recursive iframe loading by Mozilla, not in IE, because iframe's location is not directly available - M17rc1 [@ nsHTMLReflowState::Init]

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Martijn Wargers (dead)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7b) Gecko/20040309 Firefox/0.8.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7b) Gecko/20040309 Firefox/0.8.0+

Originally reported here (see for more details there):
http://forums.mozillazine.org/viewtopic.php?t=61888

I've made a basic testcase of the problem:
http://home.hccnet.nl/m.wargers/test/mozilla/crash/recursiveiframe/

This is the code, what causes it:
<iframe name="productcatalog" id="productcatalog"  src="page2.htm"></iframe>
directly followed by a script with this in it:
frames.productcatalog.location.replace(frames.productcatalog.location +
ocation.hash);

It seems that frames.productcatalog.location is still empty when the script
executes, that results in the recursive behavior.

Note that it sometimes crashes Mozilla. 

Reproducible: Always
Steps to Reproduce:
1.Visit website
2.
3.

Actual Results:  
The parent window gets recursively loaded into the iframe, resulting sometimes
in a crash.

Expected Results:  
Just show it like in Internet Explorer.

Comment 1

[WD]

Attachment: Stack Trace from scientific atlanta page

Console shows:

WARNING: Too many nested content frames so giving up, file
f:/mozilla_source/moz
illa/content/base/src/nsFrameLoader.cpp, line 425
WARNING: NS_ENSURE_TRUE(NS_SUCCEEDED(rv)) failed, file
f:/mozilla_source/mozilla
/content/base/src/nsFrameLoader.cpp, line 160
###!!! ASSERTION: failed to load URL: 'NS_SUCCEEDED(rv)', file
f:/mozilla_source
/mozilla/content/html/content/src/nsHTMLIFrameElement.cpp, line 318
WARNING: Too many nested content frames so giving up, file
f:/mozilla_source/moz
illa/content/base/src/nsFrameLoader.cpp, line 425
WARNING: NS_ENSURE_TRUE(NS_SUCCEEDED(rv)) failed, file
f:/mozilla_source/mozilla
/content/base/src/nsFrameLoader.cpp, line 339
WARNING: NS_ENSURE_TRUE(NS_SUCCEEDED(rv)) failed, file
f:/mozilla_source/mozilla
/layout/html/document/src/nsFrameFrame.cpp, line 631
WARNING: NS_ENSURE_TRUE(NS_SUCCEEDED(rv)) failed, file
f:/mozilla_source/mozilla
/layout/html/document/src/nsFrameFrame.cpp, line 288
###!!! ASSERTION: InitAndRestoreFrame failed: 'NS_SUCCEEDED(rv)', file
f:/mozill
a_source/mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp, line 4754
JavaScript error:
http://www.scientificatlanta.com/products/customers/catalog.htm line 218:
frames
.productcatalog has no properties

Comment 2

[R.K.Aa.]

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

Comment 3

[Adam Hauner]

Is there any bug for that crash? 1.7b/W2K just crashed on testcase.

Comment 4

[R.K.Aa.]

*** This bug has been marked as a duplicate of 228829 ***

Comment 5

[Martijn Wargers (dead)]

I don't think this is a duplicate.
The problem is that some javascript in Mozilla is handled wrong. 
See comment 1.
frames.productcatalog.location is still empty just after the frame
name=productcatalog is created. That's the cause of the recursive behavior,
which should not happen. 
Compare with Internet Explorer or Opera to see what should happen.
In bug 228829 I don't see any mentioning of this, that bug seems to be about the
handling of a lot of frames.

Comment 6

[R.K.Aa.]

i reopened it indeed because of the JS aspec. But a fix to prevent recursive
frames might very well fix this one too. The stack indicate a crash in frames /
nsFrameLoader - not in JS code.

Comment 7

[Boris Zbarsky [:bzbarsky]]

I'm not so sure .location should point to the iframe's source document if that
document hasn't even begun to load yet....  which is the case here.  Claiming
that the location of the frame is that document would be rather duplicitous.

Comment 8

[Boris Zbarsky [:bzbarsky]]

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

Comment 9

[Boris Zbarsky [:bzbarsky]]

The exact place we crash will vary, since this is a stack overflow crash, so
putting it in the summary is not useful.... it'll be different for each crash,
more or less.

If someone can determine exactly what IE's behavior is with regard to iframes
and reading their location, that would be much appreciated....

Comment 10

[Martijn Wargers (dead)]

New testcase:
http://home.hccnet.nl/m.wargers/test/mozilla/crash/recursiveiframe2/

This is what Internet Explorer will show:
iframe.src: page2.htm
frame location:
http://home.hccnet.nl/m.wargers/test/mozilla/crash/recursiveiframe2/page2.htm
frame location.href:
http://home.hccnet.nl/m.wargers/test/mozilla/crash/recursiveiframe2/page2.htm

This is what Mozilla will show:
iframe.src:
http://home.hccnet.nl/m.wargers/test/mozilla/crash/recursiveiframe2/page2.htm
frame location:
frame location.href:

Opera 7.23 will show this:
Inline script thread
Error:
name: TypeError
message: Statement on line 3: Expression evaluated to null or undefined and is
not convertible to Object: frames.productcatalog
Backtrace:
  Line 3 of inline#1 script in
http://home.hccnet.nl/m.wargers/test/mozilla/crash/recursiveiframe2/
    var temp = "iframe.src: " + document.getElementById("productcatalog").src +
"<br>frame location: " + frames.productcatalog.location + "<br>frame
location.href: " + frames.productcatalog.location.href;

But when it is loaded from cache, Opera will show this:
iframe.src:
http://home.hccnet.nl/m.wargers/test/mozilla/crash/recursiveiframe2/page2.htm
frame location:
http://home.hccnet.nl/m.wargers/test/mozilla/crash/recursiveiframe2/page2.htm
frame location.href:
http://home.hccnet.nl/m.wargers/test/mozilla/crash/recursiveiframe2/page2.htm

HTH

Comment 11

[Boris Zbarsky [:bzbarsky]]

Question.  In IE, if you click on a link in an iframe, is the iframe's "src"
changed, along with its location?  That is, do "src" and location always point
to the same URI?  Note that in Mozilla they currently do not.

Comment 12

[Martijn Wargers (dead)]

(In reply to comment #11)
> Question.  In IE, if you click on a link in an iframe, is the iframe's "src"
> changed, along with its location?  That is, do "src" and location always point
> to the same URI?  Note that in Mozilla they currently do not.

No, in IE the iframe's src stays page2.htm (as in Mozilla it stays
http://home.hccnet.nl/m.wargers/test/mozilla/crash/recursiveiframe3/page2.htm)

Just to make sure I understood the question, here is the testcase I was using:
http://home.hccnet.nl/m.wargers/test/mozilla/crash/recursiveiframe3/

Comment 13

[Boris Zbarsky [:bzbarsky]]

OK.

So then the real problem is that load starts are sync in IE and async in
Mozilla... so in fact, in IE when that script is running the iframe already has
a partially loaded new document, whereas in Mozilla it does not (and hence the
location is different)...

Comment 14

[Jay Patel [:jay]]

This has been a topcrasher for Mozilla 1.7beta and continues to show up in
Talkback data for rc1.  Adding M17rc1 and topcrash keyword for tracking.

Comment 15

[Jay Patel [:jay]]

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

Comment 16

[Mike Kaply [:mkaply]]

Is there anything we can do about this for 1.7? At least prevent the crash somehow?

Comment 17

[Boris Zbarsky [:bzbarsky]]

Well... We could pull out the recursion checks in nsFrameLoader::LoadURL and put
them in docshell instead so that they are done on _all_ loads in subframes, not
just loads from changes to the "src" attribute, perhaps?

Comment 18

[chris hofmann]

if it's going to make the 1.7 train we need a patch soon...

Comment 19

[Boris Zbarsky [:bzbarsky]]

Actually, never mind.  The checks _are_ being done in nsFrameLoader::LoadFrame()
and we _are_ bailing out of there.  Looking into this....

Comment 20

[Boris Zbarsky [:bzbarsky]]

So the problem is that LoadFrame returns NS_ERROR_UNEXPECTED, which is
propagated back to nsSubDocumentFrame::Init, which then returns failure.  This
makes the frame constructor somewhat unhappy and in fact we crash in reflow
because an nsTableCellFrame doesn't have a child and assumes it always will (so
it passes a null frame to a reflow state constructor).

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: Bullet-proofing

This adds some much-needed error checks to table frame construction...

This doesn't make our rendering "correct" (separate bug on that, perhaps?), but
it does keep us from crashing.

The reason I didn't just change nsFrameLoader is that that _should_ throw an
error in this case when the depth gets too big -- it won't even create a
docshell then (this is dying in nsFrameLoader::EnsureDocShell, not in
LoadFrame, looks like).  Arguably, LoadFrame should return NS_OK on nesting
limit, but doing that wouldn't stop this crash, and returning NS_OK from
EnsureDocShell without ensuring one is evil.  I suppose we could make up a new
error code that nsSubDocumentFrame::Init would special-case, but
nsSubDocumentFrame depends on getting a docshell from the frame loader (eg in
reflow) and so it makes sense for its Init() to fail in this case.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 147846 [details] [diff] [review]
Bullet-proofing

roc, this is actually pretty straightforward, as patches to this code go:

1)  Make sure to clean up allocated objects on error and return null.

2)  Make sure to check rv before assuming the return value is something you can
use.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Checked in fix for crash.  It may make sense to spin off a separate bug on the
"correctness" issue, though (and copy over relevant data).... there's a lot of
noise in this one.

Comment 24

[Jay Patel [:jay]]

Boris:  Did this make it onto the 1.7 branch as well as the Trunk?

Comment 25

[Asa Dotzler [:asa]]

Comment on attachment 147846 [details] [diff] [review]
Bullet-proofing

a=asa (on behalf of drivers) for checkin to 1.7

Comment 26

[Jay Patel [:jay]]

Well, http://www.scientificatlanta.com/products/customers/catalog.htm still
crashes for me in Mozilla 1.7rc2, so I guess this wasn't checked in on the 1.7
branch.  We should get that in soon so we can watch Talkback data for the next
release.

Comment 27

[Asa Dotzler [:asa]]

I don't see this on the branch. Can someone please land it there?

Comment 28

[Boris Zbarsky [:bzbarsky]]

Checked in the crash fix on the 1.7 branch.

Comment 29

[(not reading, please use [e-mail] instead)]

asa, this no longer blocks 1.7, since bz checked in the fix, but there is still
a reason to keep the bug open (or pen a new one.)

should we clear the 1.7+ flag?

Comment 30

[Mike Connor [:mconnor]]

fixed on aviary branch too

Comment 31

[Jay Patel [:jay]]

Boris:  What do you want to do with this bug?  Since the crash is fixed, should
we mark this bug fixed and open a new bug for the remaining rendering issues?

Comment 32

[Jacek Piskozub]

We should certainly drop the misleading blocing1.7+ flag.

Comment 33

[Karen Huang]

Verified as fix (no crash) on latest 1.7 branch Win06-24 build.
Changing keywords from fixed1.7 to verified1.7.
Leave this bug status "as is" until this bug be verified on trunk again...

Comment 34

[Jay Patel [:jay]]

I'm marking this fixed.  Other crashes with the same stack (and probably a
different cause) are covered in bug 243528.

Comment 35

[Jesse Ruderman]

Martijn, do you still have the testcase for this bug?