User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; da; rv:1.9.1) Gecko/20090624 Firefox/3.5 (.NET CLR 3.5.30729) Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; da; rv:1.9.1) Gecko/20090624 Firefox/3.5 (.NET CLR 3.5.30729) When you log on to any Fronter installation (used by 4 mio. users in EU) the images in the top navigation appears multiple times after upgrading to Firefox 3.5 This bug was not present in 3.11 or 3.10 Reproducible: Always Steps to Reproduce: 1. log in to Fronter using the link below: USERNAME: egh110787MELN PASSWORD: egh110787MELN 2. Take a look at the images in the top navigation bar FAST LOGON: https://fronter.com/DK-test/index.phtml?username=egh110787MELN&password=egh110787MELN&newlang=de&saveid=-1&mainurl=main.phtml&chp=&USER_SCREEN_SIZE= Actual Results: Images are displayed multiple times, hiding links behind. Making nativation impossible. Expected Results: The images should be shown only once. Take a look at the screenshot here: http://frontersupport.dk/ERRORS/firefox_bug.jpg

Works for me - Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.1pre) Gecko/20090702 Shiretoko/3.5.1pre Please check if Firefox's safe mode or a new profile change this behavior. Information about the steps can be found at http://support.mozilla.com/en-US/kb/Basic+troubleshooting

Tried both 1. Started in safe mode 2. Tried creating a new profile Same result Also tried disabling all add ons. Got confirmed now that a lot more users experience the same problem.

Yeah, I can reproduce it on Windows Vista. On trunk it behaves somewhat different (it also disappears after doubling itself). Regression range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=d19424342b43&tochange=dbf96b1b73bd So it is probably caused by Bug 491063.

> So it is probably caused by Bug 491063. Given that Martijn's testcase includes no images, seems a little weird... When I try that testcase using m-c builds, I see the doubled text start between rev 409416c625bc and rev 0f4de606acd7. Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=409416c625bc&tochange=0f4de606acd7 The text is still doubled on trunk in that testcase, unless I use the HTML5 parser.

It looks like the content sink ends up double-notifying on content here. That's really bad; we need to fix ASAP. My money, given that and the regression range, is on bug 481566.

Hmm. So I can't reproduce the issue at all on the "FAST LOGON" url from comment 0. On Martijn's testcase, my debug build of rev 11a3e4008446 is not showing the problem... But bug 491063 is in fact in my range, so maybe it really is causing the problem somehow. Olli, can you look into this?

The problem here is http://mxr-test.konigsberg.mozilla.org/bonsai/cvsblame.cgi?file=content/base/src/nsObjectLoadingContent.cpp&xrev=3084b64cf578&mark=1518#1488 Haven't yet figured out how to prevent calling that.

Hmm. Does that not trigger the sync updating its flushed counts? I would have thought it would have. Or is the problem that it's happening while the sink is in the middle of notifying? God, I look forward to just switching to Henri's parser....

I'll post this to tryserver too.

And I need to check other similar cases.

Or I need to tweak content sink's flush a bit...

This changes flushing so that it processes also leafs properly (in which case DidAddContent doesn't do anything if flushing happened just before calling it). Posted to tryserver.

Anyone who can reproduce the original bug, could you please test tryserver build https://build.mozilla.org/tryserver-builds/opettay@mozilla.com-content_sink_flush/

Tryserver build works fine (Win Vista).

What you think about this?

I'm not sure what this patch is actually doing (esp. with that ! business in the assert).... And mrbkap or henri might be better reviewers here. Also, does the XML sink need anything similar?

I'm not at all competent to review this patch, because I don't understand in detail how the old sink works. It does scare me a bit that the child is read back from the tree by index. Is it possible that a script has changed the tree by then?

(In reply to comment #17) > I'm not sure what this patch is actually doing The patch makes sure that we notify leaf tags properly when flushing. Usually leaf tags are notified in DidAddContent. > (esp. with that ! business in > the assert).... Just keeping the existing assertion to do whatever it is doing now. > Also, does the XML sink need anything similar? No, because it doesn't do similar insertion point hacks.

> The patch makes sure that we notify leaf tags properly when flushing. Yes, I gathered that. Why is this particular leaf not notified from DidAddContent? And why do the changes you made make sure to notify on it? > Just keeping the existing assertion to do whatever it is doing now. I guess !(a > b) is confusing to me. Why not a <= b? For that matter, you're just trying to guard on |stackPos + 1 <= mStackPos| before getting mStack[stackPos+1], right? Rewriting it that way would be clearer, I think...

(In reply to comment #20) > Yes, I gathered that. Why is this particular leaf not notified from > DidAddContent? And why do the changes you made make sure to notify on it? DidAddContent renotifies, but not the right thing, because flush (which doesn't work properly) is called before it. (I know, not the best situation to be, but I think one could get into similar problem also elsewhere.) Other option could be to add script blockers high enough in content sink.

> DidAddContent renotifies, but not the right thing, because flush (which doesn't > work properly) is called before it. So with your patch, is that issue fixed? If so, why? > Other option could be to add script blockers high enough in content sink. I'm not sure I follow.... Maybe the problem here is just a lack of a description of what goes wrong when the bug appears. If you have that info available, can you summarize?

The problem is that flush is called between binding the element to document and DidAddContent. And flush doesn't handle leaf tags properly. If flush worked properly, DidAddContent doesn't do anything because the content is already notified.

> The problem is that flush is called between binding the element to document > and DidAddContent. Aha! That makes things clear. Sorry for the lag here; I needed to sit down and relearn this code a bit...

Comment on attachment 386995 [details] [diff] [review] +better test >+++ b/content/html/document/src/nsHTMLContentSink.cpp >+ if (mStack[stackPos].mInsertionPoint != -1) { > PRInt32 childIndex = mStack[stackPos].mInsertionPoint - 1; Here, please document that we might have popped the child off our stack already but not notified on it yet, which is why we have to get it directly from its parent node. >+ NS_ASSERTION(!(mStackPos > (stackPos + 1)) || Add "// Child not on stack anymore; can't assert it's correct" to the end of the line there? With those comment changes, r=bzbarsky. Again, sorry for the terrible lag. :( I don't think this needs a second review. Please request 1.9.2 approval and get this landed on m-c ASAP? (Either when approved or when the tree reopens, whichever comes first). We need this on 1.9.1 too, right? What about 1.9.0?

Comment on attachment 394359 [details] [diff] [review] +comments a1.9.2=dbaron

Comment on attachment 394359 [details] [diff] [review] +comments Approved for 1.9.1.4, a=dveditz for release-drivers

Verified the bug using attached testcase in 1.9.1.3 and that it is fixed in 1.9.1.4 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.1.4pre) Gecko/20090929 Shiretoko/3.5.4pre.