288460 - link urls get replaced by urls appearing earlier in the page

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect)

Description

[caines]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b2) Gecko/20050330 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b2) Gecko/20050330 Firefox/1.0+

In the page www.abstractappeal.com, some of the tags in the column running down
the right hand side point to incorrect locations when the page is rendered. In
particular, the link "Fla ERISA Blog" and all the links below it point to the
same url (the url of the florida erisa blog). When the page is saved from
Firefox errors are also introduced: an extra <a> tag pointing at the florida
erisa blog is inserted, without a corresponding </a> tag.

In the page's source code the links have distinct url's, and Safari renders the
page correctly.

Reproducible: Always

Steps to Reproduce:
1. Go to www.abstractappeal.com
2. Look at the targets of the links below the "Fla ERISA Blog" link.

Actual Results:  
You see a large number of links all going to the same location.

Expected Results:  
You should see large number of links going to distinct locations of interest to
some guy in Florida.

The problem was seen with the latest builds of Mozilla, Firefox and Camino.

You can obviously work around it by copying the correct urls out of the page
source, but it's annoying.

Comment 1

[caines]

Oops. Looking again I see that abstractappeal.com is missing an </a> tag, so the
page is bad html. But, still, is there a reason it's bad to implicitly close out
a previous <a> tag when a new one opens?

Comment 2

[Boris Zbarsky [:bzbarsky]]

The site works fine in a mid-April trunk build... did the page change?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Minimal testcase based on mail from reporter

Comment 4

[Boris Zbarsky [:bzbarsky]]

Blake, do we have a bug on this already?

The problem, of course, is that <a> may contain <font> and <font> may contain
<div> and <div> may contain <a> (but <a> may not contain <div> or <a>, but by
the time we get there we'd have to close out the font too and that gets messy).
 In any case, in the content model we have:

a
  font
    div
      a

and due to other bugs we have the click goes to the URL of the outer <a>...

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

I don't know offhand of an existing bug that describes this.

What's happening is that my fix for bug 90664 wasn't strong enough to catch this
case. Perhaps ScanDocStructure() needs to be more thorough in marking all
children of the outer "malformed" tag as malformed.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v1

What's happening here is that the <font> is "well formed" as far as the parser
could tell, so we're allowing it to contain the <div>. This means that we
implicitly allow the <a> to contain the <div>, and are unable to close it when
we reach the inner <a>. With this patch, once we see a case such as
<a><font><div><a>, we mark the outer <a> and all of its children (including the
<font>) as being malformed, which allows us to perform RS handling on it.

I had to export nsDequeIterator to get this patch to link. I could write a
version that doesn't use it, but I think this is cleaner.

Comment 7

[rbs]

Comment on attachment 183743 [details] [diff] [review]
patch v1

r=rbs, the idea seems reasonnable to me.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 183743 [details] [diff] [review]
patch v1

>Index: src/nsHTMLTokenizer.cpp

>+                  // all. Mark the previous one and all of its children as 
>+                  // malformed to increase our chances of doing RS handling
>+                  // all of them. We want to do this for cases 

"on all of them", perhaps?

such as:
>+                  // <a><div><a></a></div></a>.
>+                  // Note that we have to iterate through all of the chilren
>+                  // of the original malformed tag to protect against:
>+                  // <a><font><div><a></a></div></font></a>, so that the <font>
>+                  // is allowed to contain the <div>.
>                   // XXX What about <a><span><a>, where the second <a> closes
>                   // the <span>?
>-                  CHTMLToken *theMalformedToken = 
>-                    NS_STATIC_CAST(CHTMLToken*, theStack.ObjectAt(earlyPos));
>+                  nsDequeIterator it(theStack, earlyPos);
>+                  while (it < theStack.End()) {
>+                    CHTMLToken *theMalformedToken = 
>+                        NS_STATIC_CAST(CHTMLToken*, it++);
>                   
>-                  theMalformedToken->SetContainerInfo(eMalformed);
>+                    theMalformedToken->SetContainerInfo(eMalformed);
>+                  }
>                 }
>               }
> 
>               theStack.Push(theToken);
>               ++theStackDepth;
>             }
>             break;
>           case eToken_end: 
>             {
>               CHTMLToken *theLastToken = NS_STATIC_CAST(CHTMLToken*, theStack.Peek());
>Index: ../../xpcom/ds/nsDeque.h
>===================================================================
>RCS file: /cvsroot/mozilla/xpcom/ds/nsDeque.h,v
>retrieving revision 3.30
>diff -u -1 -0 -p -r3.30 nsDeque.h
>--- ../../xpcom/ds/nsDeque.h	18 Apr 2004 14:18:13 -0000	3.30
>+++ ../../xpcom/ds/nsDeque.h	16 May 2005 18:11:10 -0000
>@@ -238,21 +238,21 @@ private:
>    */
>   nsDeque& operator=(const nsDeque& anOther);
> 
>   PRInt32 GrowCapacity();
> };
> 
> /******************************************************
>  * Here comes the nsDequeIterator class...
>  ******************************************************/
> 
>-class nsDequeIterator {
>+class NS_COM nsDequeIterator {
> public:
>   /**
>    * DequeIterator is an object that knows how to iterate
>    * (forward and backward) through a Deque. Normally,
>    * you don't need to do this, but there are some special
>    * cases where it is pretty handy.
>    *
>    * One warning: the iterator is not bound to an item,
>    * it is bound to an index, so if you insert or remove
>    * from the beginning while using an iterator
>Index: ../../xpcom/build/dlldeps.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/xpcom/build/dlldeps.cpp,v
>retrieving revision 1.124
>diff -u -1 -0 -p -r1.124 dlldeps.cpp
>--- ../../xpcom/build/dlldeps.cpp	27 Apr 2005 01:38:35 -0000	1.124
>+++ ../../xpcom/build/dlldeps.cpp	16 May 2005 18:11:11 -0000
>@@ -116,20 +116,21 @@ void XXXNeverCalled()
>     NS_OutputStreamIsBuffered(nsnull);
>     PL_DHashStubEnumRemove(nsnull, nsnull, nsnull, nsnull);
>     nsIDHashKey::HashKey(nsnull);
>     nsFixedSizeAllocator a;
>     nsRecyclingAllocator recyclingAllocator(2);
>     a.Init(0, 0, 0, 0, 0);
>     a.Alloc(0);
>     a.Free(0, 0);
>     nsIThread::GetCurrent(nsnull);
>     nsDeque(nsnull);
>+    nsDequeIterator(nsDeque());
>     nsTraceRefcnt::LogAddCOMPtr(nsnull, nsnull);
>     nsTraceRefcntImpl::DumpStatistics();
>     NS_NewEmptyEnumerator(nsnull);
>     new nsArrayEnumerator(nsnull);
>     NS_QuickSort(nsnull, 0, 0, nsnull, nsnull);
>     nsString();
>     NS_ProxyRelease(nsnull, nsnull, PR_FALSE);
>     XPT_DoString(nsnull, nsnull, nsnull);
>     XPT_DoHeader(nsnull, nsnull, nsnull);
> #if defined (DEBUG) && !defined (WINCE)

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 183743 [details] [diff] [review]
patch v1

This is a fairly safe fix that wallpapers over some weird event handling bugs.

Comment 10

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 183743 [details] [diff] [review]
patch v1

a=shaver

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked in.

Comment 12

[David G King]

I'm not 100% sure, but I think this caused the following on Win32/MinGW/cygwin :-

e:/mozilla/source/mozilla/xpcom/build/../ds/nsDeque.h: In function `void XXXNeve
rCalled()':
e:/mozilla/source/mozilla/xpcom/build/../ds/nsDeque.h:231: error: `nsDeque::nsDe
que(const nsDeque&)' is private
e:/mozilla/source/mozilla/xpcom/build/dlldeps.cpp:131: error: within this contex
t
make[4]: *** [dlldeps.o] Error 1
make[4]: Leaving directory `/cygdrive/e/mozilla/source/mozilla/obj/mozilla/xpcom
/build'
make[3]: *** [libs] Error 2
make[3]: Leaving directory `/cygdrive/e/mozilla/source/mozilla/obj/mozilla/xpcom
'

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

It did. I checked in a bustage fix, and the t'boxes that were complaining are
now green again. Marking FIXED.

Comment 14

[David G King]

I've just pulled from the trunk and rebuilt, and still get the same error.

It's not Fixed.

(In reply to comment #13)
> It did. I checked in a bustage fix, and the t'boxes that were complaining are
> now green again. Marking FIXED.

Comment 15

[timeless]

2005-06-03 08:00
mozilla/xpcom/build/dlldeps.cpp 	1.130

patch by mrbkap (on irc), r=me (on irc), sr=dmose (on irc), a=shaver (on irc)

Comment 16

[David G King]

Confirming that the patch has fixed by MinGW build problems. Thanks guys.