Closed Bug 237760 Opened 19 years ago Closed 18 years ago

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

Categories

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

x86
Windows 2000
defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: martijn.martijn, Unassigned)

References

()

Details

(Keywords: crash, topcrash, verified1.7, Whiteboard: bz landed the fix on the trunk and 1.7 branch, fixed-aviary1.0)

Crash Data

Attachments

(2 files)

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.
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
*** Bug 238069 has been marked as a duplicate of this bug. ***
Is there any bug for that crash? 1.7b/W2K just crashed on testcase.

*** This bug has been marked as a duplicate of 228829 ***
Status: UNCONFIRMED → RESOLVED
Closed: 19 years ago
Resolution: --- → DUPLICATE
Status: RESOLVED → UNCONFIRMED
Resolution: DUPLICATE → ---
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.
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.
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.
*** Bug 241231 has been marked as a duplicate of this bug. ***
Severity: normal → critical
Keywords: crash
Summary: Recursive iframe loading by Mozilla, not in IE, because iframe's location is not directly available → Recursive iframe loading by Mozilla, not in IE, because iframe's location is not directly available [@ nsHTMLReflowState::Init ]
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....
Summary: Recursive iframe loading by Mozilla, not in IE, because iframe's location is not directly available [@ nsHTMLReflowState::Init ] → Recursive iframe loading by Mozilla, not in IE, because iframe's location is not directly available
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
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.
(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/
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)...
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: topcrash
Summary: Recursive iframe loading by Mozilla, not in IE, because iframe's location is not directly available → Recursive iframe loading by Mozilla, not in IE, because iframe's location is not directly available - M17rc1 [@ nsHTMLReflowState::Init]
*** Bug 234301 has been marked as a duplicate of this bug. ***
Is there anything we can do about this for 1.7? At least prevent the crash somehow?
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?
if it's going to make the 1.7 train we need a patch soon...
Actually, never mind.  The checks _are_ being done in nsFrameLoader::LoadFrame()
and we _are_ bailing out of there.  Looking into this....
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).
Attached patch Bullet-proofingSplinter Review
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 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.
Attachment #147846 - Flags: superreview?(roc)
Attachment #147846 - Flags: review?(roc)
Attachment #147846 - Flags: superreview?(roc)
Attachment #147846 - Flags: superreview+
Attachment #147846 - Flags: review?(roc)
Attachment #147846 - Flags: review+
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.
Blocks: 243528
Boris:  Did this make it onto the 1.7 branch as well as the Trunk?
Flags: blocking1.7?
Comment on attachment 147846 [details] [diff] [review]
Bullet-proofing

a=asa (on behalf of drivers) for checkin to 1.7
Attachment #147846 - Flags: approval1.7+
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.
I don't see this on the branch. Can someone please land it there?
Flags: blocking1.7? → blocking1.7+
Whiteboard: patch ready to land
Whiteboard: patch ready to land → email sent to bz asking for help landing
Checked in the crash fix on the 1.7 branch.
Keywords: fixed1.7
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?
Whiteboard: email sent to bz asking for help landing → bz landed the fix on the trunk and 1.7 branch
fixed on aviary branch too
Whiteboard: bz landed the fix on the trunk and 1.7 branch → bz landed the fix on the trunk and 1.7 branch, fixed-aviary1.0
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?
We should certainly drop the misleading blocing1.7+ flag.
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...
Keywords: fixed1.7verified1.7
I'm marking this fixed.  Other crashes with the same stack (and probably a
different cause) are covered in bug 243528.
Status: NEW → RESOLVED
Closed: 19 years ago18 years ago
Resolution: --- → FIXED
Martijn, do you still have the testcase for this bug?
Flags: in-testsuite?
Crash Signature: [@ nsHTMLReflowState::Init]
You need to log in before you can comment on or make changes to this bug.