Closed Bug 855443 Opened 11 years ago Closed 10 years ago

Assertion failure in nsSHEntry.cpp:595 - multiple frames with multiple children

Categories

(Core :: DOM: Navigation, defect)

21 Branch
x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: tanvi, Assigned: smaug)

References

Details

Attachments

(3 files, 1 obsolete file)

The mochitests here https://bugzilla.mozilla.org/attachment.cgi?id=729696&action=edit caused this assert to fail: http://mxr.mozilla.org/mozilla-central/source/docshell/shistory/src/nsSHEntry.cpp#595 - NS_ERROR("Adding a child where we already have a child? This may misbehave");

Here is a stack trace: http://www.pastebin.mozilla.org/2251254

Talking to Olli, it sounds like the mochitest discovered an edge case that wasn't handled in bug 462076 (or one of it's related bugs).

The mochitest involves the following:
Top level Page 1 has Child Iframe 2A and Child Iframe 2B (created with an <iframe> html tag; not dynamically).

Child Iframe 2A creates grandchild iframe 3A and grandchild iframe 3B dynamically.
Grandchild Iframe 3B contains great grandchild iframe 4A (through an <iframe> tag).
Grandchild Iframe 3A and great grandchild iframe 4A navigate to another location through a dynamic link click.

Child Iframe 2B behaves the same way as Child Iframe 2A.  More specifically...

Child Iframe 2B creates grandchild iframe 3a and grandchild iframe 3b dynamically.
Grandchild Iframe 3b contains great grandchild iframe 4a (through an <iframe> tag).
Grandchild Iframe 3a and great grandchild iframe 4a navigate to another location through a dynamic link click.
Blocks: 901876
I'm hitting this assertion too with the tests for bug 704320 (meta referrer).
Blocks: 704320
Olli, any thoughts on what might be going on here?
Sid, do you perhaps have a minimal testcase for this?
Flags: needinfo?(sstamm)
I don't have a minimal testcase yet, though the big crazy tests in bug 704320 hit it (I know, not ideal).

I'm out of the office for the next few days but can try to make a minimal test case when I get back.  Given on my brief chat with Tanvi, I'm guessing one can be created by making a bunch of iframes that each have a bunch of children.  Not clearing the needinfo here so I remember to check back in when I return.
Okay, so I was able to significantly reduce the tests from bug 704320 into some static files that cause this assertion.  I notice it doesn't happen all the time, but it's pretty reliable on my local debug build/x86_64.

Key elements:
* Form with a submit (navigation, probably the important bit)
* style @import
* background set via CSS
* iframes with iframes

Not sure the root cause, but this should be able to trigger the assertion for you.
Flags: needinfo?(sstamm)
Thanks Sid!

Adding to my ni list so that I don't forget to test.
Flags: needinfo?(bugs)
Hey Olli, have you had a chance to look into this yet?
Not yet, sorry.
Flags: needinfo?(bugs)
Flags: needinfo?(bugs)
Attached patch WIPSplinter Review
https://tbpl.mozilla.org/?tree=Try&rev=292d311fbace

Sid or Tanvi, could you test the patch. The assertion happens only occasionally, but with the patch I haven't managed to trigger it.
Assignee: nobody → bugs
Attachment #8504218 - Flags: review?(sstamm)
Flags: needinfo?(bugs)
Attachment #8504218 - Flags: review?(sstamm) → feedback?(sstamm)
Looks like the patch fixes quite a few cases like
Assertion count 0 is less than expected range 23-23 assertions on try.
Comment on attachment 8504218 [details] [diff] [review]
WIP

Review of attachment 8504218 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, seems to work.  I applied this patch and ran my tests a few times... no assertions.  They popped up pretty reliably (3-5 times per run) before, so I think you found the problem.
Attachment #8504218 - Flags: feedback?(sstamm) → feedback+
Attached patch +test fixes (obsolete) — Splinter Review
Looks like we can remove quite a few SimpleTest.expectAssertions() calls.

https://tbpl.mozilla.org/?tree=Try&rev=c91be3e39b9a
Flags: needinfo?(bugs)
Attached patch +test fixes, v2Splinter Review
Some more test fixes

https://tbpl.mozilla.org/?tree=Try&rev=7fed1165f237
Attachment #8504290 - Attachment is obsolete: true
Comment on attachment 8504346 [details] [diff] [review]
+test fixes, v2

This is a bit scary change given that any changes to session history behavior tend to regress something.
Attachment #8504346 - Flags: review?(bzbarsky)
Flags: needinfo?(bugs)
Comment on attachment 8504346 [details] [diff] [review]
+test fixes, v2

(IIRC mconley wanted at least at some point to get use to docshell code.)
Attachment #8504346 - Flags: feedback?(mconley)
Comment on attachment 8504346 [details] [diff] [review]
+test fixes, v2

r=me.  Scary...
Attachment #8504346 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/e3e717e9b628
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Comment on attachment 8504346 [details] [diff] [review]
+test fixes, v2

Review of attachment 8504346 [details] [diff] [review]:
-----------------------------------------------------------------

I'm a bit late to the game here, but from what I know about nsDocShell and the session history stuff (and how they related to nested frames), I think this makes sense. I agree though, this kinda scary stuff. :)
Attachment #8504346 - Flags: feedback?(mconley) → feedback+
Depends on: 1090918
Depends on: 1244517
You need to log in before you can comment on or make changes to this bug.