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)

Product:
Core
Component:
Layout
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9.3a1

People

(Reporter: jruderman, Assigned: tnikkel)

References

Details

(Keywords: assertion, regression, testcase)

Attachments

(4 files, 3 obsolete files)

testcase
261 bytes, application/xhtml+xml
Details
testcase2
747 bytes, text/html
Details
patch v3
9.52 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
fix up some failing tests v2
14.88 KB, patch
enndeakin
: review+
Details | Diff | Splinter Review
Attached file testcase — 
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.
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


and:

###!!! ASSERTION: Found more undisplayed content data after removal: 'context == nsnull', file /Users/jruderman/trunk/mozilla/layout/base/nsFrameManager.cpp, line 628


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-
Attached file testcase2 — 
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+
This interferes with fuzzing by triggering a lot of different assertions.
Timothy has been working on something that might fix this bug too, iirc.
My patch in bug 497519 fixes the assertion in Jesse's testcase. But Martijn's testcase is a different issue.
Attached patch patch (obsolete) — — Splinter Review 
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?
Attachment #387844 - Flags: review?(jonas)
Blocks: 506349
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.
Attached patch patch v2 (obsolete) — — Splinter Review 
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.
Attached patch patch v3 — — Splinter Review 
Needed to add a weak frame check.
Attachment #399012 - Attachment is obsolete: true
Attachment #399132 - Flags: review?(bzbarsky)
Attachment #399012 - Flags: review?(bzbarsky)
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+
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)?
Attached patch fix up some failing tests (obsolete) — — Splinter Review 
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 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.
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)
Attachment #410707 - Flags: review?(enndeakin) → review+
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.
There's no sane reason changes to this code would affect xpcshell tests...
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.
(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.
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
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.

Attachment

General

Creator:
Created:
Updated:
Size: