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)
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: tanvi, Assigned: smaug)
References
Details
Attachments
(3 files, 1 obsolete file)
3.87 KB,
patch
|
Details | Diff | Splinter Review | |
869 bytes,
patch
|
geekboy
:
feedback+
|
Details | Diff | Splinter Review |
11.51 KB,
patch
|
bzbarsky
:
review+
mconley
:
feedback+
|
Details | Diff | Splinter Review |
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.
Comment 1•10 years ago
|
||
I'm hitting this assertion too with the tests for bug 704320 (meta referrer).
Blocks: 704320
Comment 2•10 years ago
|
||
Olli, any thoughts on what might be going on here?
Assignee | ||
Comment 3•10 years ago
|
||
Sid, do you perhaps have a minimal testcase for this?
Flags: needinfo?(sstamm)
Comment 4•10 years ago
|
||
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.
Comment 5•10 years ago
|
||
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)
Assignee | ||
Comment 6•10 years ago
|
||
Thanks Sid! Adding to my ni list so that I don't forget to test.
Flags: needinfo?(bugs)
Comment 7•10 years ago
|
||
Hey Olli, have you had a chance to look into this yet?
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(bugs)
Assignee | ||
Comment 9•10 years ago
|
||
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 | ||
Updated•10 years ago
|
Attachment #8504218 -
Flags: review?(sstamm) → feedback?(sstamm)
Assignee | ||
Comment 10•10 years ago
|
||
Looks like the patch fixes quite a few cases like Assertion count 0 is less than expected range 23-23 assertions on try.
Comment 11•10 years ago
|
||
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+
Assignee | ||
Comment 12•10 years ago
|
||
Looks like we can remove quite a few SimpleTest.expectAssertions() calls. https://tbpl.mozilla.org/?tree=Try&rev=c91be3e39b9a
Flags: needinfo?(bugs)
Assignee | ||
Comment 13•10 years ago
|
||
Some more test fixes https://tbpl.mozilla.org/?tree=Try&rev=7fed1165f237
Attachment #8504290 -
Attachment is obsolete: true
Assignee | ||
Comment 14•10 years ago
|
||
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)
Assignee | ||
Comment 15•10 years ago
|
||
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 16•10 years ago
|
||
Comment on attachment 8504346 [details] [diff] [review] +test fixes, v2 r=me. Scary...
Attachment #8504346 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 17•10 years ago
|
||
Good time to land, early in a cycle. https://hg.mozilla.org/integration/mozilla-inbound/rev/e3e717e9b628
Comment 18•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e3e717e9b628
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Comment 20•10 years ago
|
||
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+
You need to log in
before you can comment on or make changes to this bug.
Description
•