Closed Bug 502168 Opened 12 years ago Closed 12 years ago

Duplicated content with <embed> inside table markup

Categories

(Core :: Layout, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.2 --- beta1-fixed
blocking1.9.1 --- .4+
status1.9.1 --- .4-fixed

People

(Reporter: thomas, Assigned: smaug)

References

()

Details

(Keywords: regression, testcase, verified1.9.1, Whiteboard: [sg:critical?])

Attachments

(4 files, 1 obsolete file)

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.
Blocks: 491063
Status: UNCONFIRMED → NEW
Component: General → DOM
Ever confirmed: true
Keywords: regression
Product: Firefox → Core
QA Contact: general → general
Version: unspecified → Trunk
Attached file testcase
Group: core-security
Component: DOM → Layout
Flags: blocking1.9.1?
QA Contact: general → layout
Keywords: testcase
> 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.
Blocks: 481566
No longer blocks: 491063
Flags: blocking1.9.1.1?
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?
Assignee: nobody → Olli.Pettay
Blocks: 491063
No longer blocks: 481566
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...
Attached patch better (obsolete) — Splinter Review
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).
Attached patch +better testSplinter Review
What you think about this?
Attachment #386974 - Attachment is obsolete: true
Attachment #386995 - Flags: review?(bzbarsky)
Flags: blocking1.9.1?
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.
Attachment #386995 - Flags: review?(mrbkap)
> 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.
blocking1.9.1: --- → needed
Flags: blocking1.9.1.1? → blocking1.9.1.1-
> 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?
Attachment #386995 - Flags: review?(mrbkap)
Attachment #386995 - Flags: review?(bzbarsky)
Attachment #386995 - Flags: review+
Attached patch +commentsSplinter Review
http://hg.mozilla.org/mozilla-central/rev/5d0c03667020
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Attachment #394359 - Flags: approval1.9.2?
Comment on attachment 394359 [details] [diff] [review]
+comments

a1.9.2=dbaron
Attachment #394359 - Flags: approval1.9.2? → approval1.9.2+
Flags: in-testsuite?
Flags: in-testsuite? → in-testsuite+
blocking1.9.1: needed → .4+
Attachment #394359 - Flags: approval1.9.1.4?
Whiteboard: [sg:critical?]
Flags: wanted1.9.0.x-
Comment on attachment 394359 [details] [diff] [review]
+comments

Approved for 1.9.1.4, a=dveditz for release-drivers
Attachment #394359 - Flags: approval1.9.1.4? → approval1.9.1.4+
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.
Keywords: verified1.9.1
Depends on: 525276
Group: core-security
Summary: Particular images are displayed multiple times in a formated way - only FF 3.5 → Duplicated content with <embed> inside table markup
You need to log in before you can comment on or make changes to this bug.