Closed
Bug 396367
Opened 17 years ago
Closed 15 years ago
"ASSERTION: Already have an undisplayed context entry for aContent" with marquee, frameset, area
Categories
(Core :: Layout, defect)
Tracking
()
RESOLVED
FIXED
mozilla1.9.3a1
People
(Reporter: jruderman, Assigned: tnikkel)
References
Details
(Keywords: assertion, regression, testcase)
Attachments
(4 files, 3 obsolete files)
(deleted),
application/xhtml+xml
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
enndeakin
:
review+
|
Details | Diff | Splinter Review |
Loading the testcase triggers:
###!!! ASSERTION: Already have an undisplayed context entry for aContent: '!GetUndisplayedContent(aContent)', file /Users/jruderman/tclean/mozilla/layout/base/nsFrameManager.cpp, line 571
I think this is a regression from within the last week.
Reporter | ||
Comment 1•17 years ago
|
||
I think this bug can lead to:
###!!! ASSERTION: node in map twice: 'Not Reached', file /Users/jruderman/trunk/mozilla/layout/base/nsFrameManager.cpp, line 1679
Reporter | ||
Comment 2•17 years ago
|
||
and:
###!!! ASSERTION: Found more undisplayed content data after removal: 'context == nsnull', file /Users/jruderman/trunk/mozilla/layout/base/nsFrameManager.cpp, line 628
Comment 3•17 years ago
|
||
The issue here is that the undisplayed map stores the undisplayed entry keyed by content parent. In this case, for the <area> that's the <marquee>. But when we're reframing the frameset, we walk up to the closest containing block because of the {ib} split and reframe that. That's a div inside the marquee (which is a flattened tree parent of the <area> due to the magic of XBL). We clear out undisplayed content for everything that's got that div as a parent... which does not include the <area>. Then we go to reconstruct the area, add another undisplayed entry, and get the assertion.
David, how serious is this assertion? Do we need to worry about fixing this for 1.9?
Flags: blocking1.9?
Flags: wanted1.9+
Flags: blocking1.9?
Flags: blocking1.9-
Comment 4•17 years ago
|
||
I'm seeing this assertion and the 'node in map twice' assertion with this testcase in current debug trunk build.
It makes use of enhanced privileges, so you need to download it to your computer.
Flags: wanted1.9.0.x+
Flags: wanted1.9-
Flags: wanted1.9+
Reporter | ||
Comment 5•15 years ago
|
||
This interferes with fuzzing by triggering a lot of different assertions.
Comment 6•15 years ago
|
||
Timothy has been working on something that might fix this bug too, iirc.
Assignee | ||
Comment 7•15 years ago
|
||
My patch in bug 497519 fixes the assertion in Jesse's testcase. But Martijn's testcase is a different issue.
Assignee | ||
Comment 8•15 years ago
|
||
This is a patch for the assertion in Martijn's testcase.
When we execute the second script element, the second script element's content node is in the tree but we haven't yet done the NotifyAppend/Insert. Then when we set the textzoom we RebuildAllStyleData, and RecreateFramesForContent the body, and we eventually ProcessChildren the children of the body, including the content node for the second script element since it's in the tree. Then when RebuildAllStyleData finishes the script blocker exits, it is the last one, so we run pending scripts, there is a DelayedEditorInit pending for the input element, which calls FlushPendingNotifications, and that calls FlushTags. FlushTags then does NotifyAppend/Insert for the second script element, but we already processed it.
Martijn and Jesse, if you are still getting these asserts with this patch and the above mentioned patch applied please let me know and cc me on any future bugs for these asserts.
Boris, or anyone else, who is the proper reviewer for this patch?
Assignee | ||
Updated•15 years ago
|
Attachment #387844 -
Flags: review?(jonas)
This won't help if the same code is run from a time-out, does it? I.e. what if the <script> lived inside the <head>, but executed the same code using setTimeout. It'll be much trickier to hit, but it seems like it can still happen.
Should the flush call live in RebuildAllStyleData and/or RecreateFramesForContent instead?
Of course, the best fix is if we don't to this lazy notification thing at all. We should do that after the HTML5 is preffed on, unless it already does it.
Assignee | ||
Comment 11•15 years ago
|
||
Good point about the setTimeout. So I guess we need to make sure that we have flushed anytime we process restyles that might cause frame construction to happen.
The second test is the testcase from bug 506349.
Mats' somewhat similar patch in bug 506349 fails a mochitest (I checked that this patch does not), but I'd feel more comfortable if this got a spin on try server.
Assignee: nobody → tnikkel
Attachment #387844 -
Attachment is obsolete: true
Attachment #399012 -
Flags: review?(bzbarsky)
Attachment #387844 -
Flags: review?(jonas)
You should get hg access, if you want it.
Assignee | ||
Comment 13•15 years ago
|
||
Needed to add a weak frame check.
Attachment #399012 -
Attachment is obsolete: true
Attachment #399132 -
Flags: review?(bzbarsky)
Attachment #399012 -
Flags: review?(bzbarsky)
Comment 14•15 years ago
|
||
Comment on attachment 399132 [details] [diff] [review]
patch v3
I think you need a kungFuDeathGrip on |this| in PresShell::ReconstructFrames. Other than that, looks ok.
Attachment #399132 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 15•15 years ago
|
||
I think this is causing failures on try server in test_showcaret.xul, for example
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1256853322.1256863422.12849.gz
where we have the failure "scrollY for showcaret - got 15, expected 0".
I tried adding waitForFocus, but still get the error. The scrollY has always been small, like 27 or 15, so the caret should still be in view, which is what I think the test is actually testing. Neil, can I change the test to check that scrollY is less than say 50 (half the height of the iframe)?
Assignee | ||
Comment 16•15 years ago
|
||
Here are the changes I would like to make to tests so that this bug doesn't make them fail.
Attachment #409831 -
Flags: review?(enndeakin)
Comment 17•15 years ago
|
||
Comment on attachment 409831 [details] [diff] [review]
fix up some failing tests
>- is(frames[1].scrollY, 0, "scrollY for showcaret");
>+ ok(frames[1].scrollY < 50, "scrollY for showcaret");
Looks ok. I'm not sure why it was checking for 0; this patch presumably flushes earlier and makes this correct.
The caret should be at the end of the text (after the word Goodbye) 50 should be large enough on most systems. You could check if it was smaller than the Goodbye text rectangle though, but no big deal.
> function onLoad()
> {
>+ SimpleTest.waitForFocus(runtest);
>+}
>+
>+function runtest()
>+{
waitForFocus already handles loading, so just put the call to it at the end of the script outside of any functions, and remove the load event listener.
Same with the other tests.
Assignee | ||
Comment 18•15 years ago
|
||
Changes:
Compare the scroll to the rect of "Goodbye".
Got rid of onload handler and just use waitForFocus.
Attachment #409831 -
Attachment is obsolete: true
Attachment #410707 -
Flags: review?(enndeakin)
Attachment #409831 -
Flags: review?(enndeakin)
Updated•15 years ago
|
Attachment #410707 -
Flags: review?(enndeakin) → review+
Assignee | ||
Comment 19•15 years ago
|
||
Pushed the test fixups as
http://hg.mozilla.org/mozilla-central/rev/748164283b68
Pushed the main patch as
http://hg.mozilla.org/mozilla-central/rev/63d4a49fbec1
And push a change to the tests to that they reset the zoom when finished
http://hg.mozilla.org/mozilla-central/rev/640bf652618a
But had to backout all but the test fixups
(merge) http://hg.mozilla.org/mozilla-central/rev/1d9ba8c553fb
http://hg.mozilla.org/mozilla-central/rev/35fec83dd2c5
http://hg.mozilla.org/mozilla-central/rev/387da5403019
due to failures in xpcshell\tests\test_satchel\unit\test_history_api.js and xpcshell\tests\test_places\bookmarks\test_360134.js
log:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1262395353.1262397461.12862.gz
I got a bunch of different random failures in xpcshell tests on Windows on try server, never the same, so I thought I might have to backout. I'll have to think about how to move this bug forward.
Comment 20•15 years ago
|
||
There's no sane reason changes to this code would affect xpcshell tests...
Assignee | ||
Comment 21•15 years ago
|
||
philor on IRC has informed me that bug 535585 covers random failures in random xpcshell tests on Windows, so that would explain why I couldn't find any filed bugs on the failures I was seeing. I will try relanding later.
Comment 22•15 years ago
|
||
(In reply to comment #21)
> philor on IRC has informed me that bug 535585 covers random failures in random
> xpcshell tests on Windows, so that would explain why I couldn't find any filed
> bugs on the failures I was seeing. I will try relanding later.
Yeah, it looks like those tests all passed, but xpcshell just returned non-zero there.
Assignee | ||
Comment 23•15 years ago
|
||
Pushed again
http://hg.mozilla.org/mozilla-central/rev/371b86097c25
http://hg.mozilla.org/mozilla-central/rev/a3423adc286e
had to star another occurrence of bug 535585. Seems odd that I would see bug 535585 so often with this patch.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 24•15 years ago
|
||
And that is the last "Already have an undisplayed context entry for aContent" assertion that I know of left. So if more are lurking please file and cc me.
Flags: in-testsuite+
Target Milestone: --- → mozilla1.9.3a1
You need to log in
before you can comment on or make changes to this bug.
Description
•