28825 - Notify function called twice (HTML) or not at all (XML) for one tag

Status: RESOLVED WONTFIX

(Core :: DOM: HTML Parser, defect, P3)

Description

[Thomas Lendacky]

Should the nsDTDUtils.cpp method CObserverService::RegisterObservers keep a 
seperate queue for NS_PARSER_SUBJECT and NS_XMLPARSER_SUBJECT?

Currently, if you perform an AddObserver for NS_PARSER_SUBJECT and 
NS_XMLPARSER_SUBJECT, to become an ElementObserver, for the same tag, "META" for 
example, you are notified twice for each tag in an HTML file and not all if it 
is in an XML file.

Since the method is using a common queue for the subjects, each registration for 
a tag in a different subject results in two entries.  So when CNavDTD.cpp 
performs the Notify call loop it results in two notifications for one tag.

For XML, there doesn't appear to be any Observer->Notify(...) call in the XML 
DTD nsWellFormedDTD.cpp module so it results on no notifications for the tag.

Comment 1

[Thomas Lendacky]

I forgot to mention the level of the build.  This has been confirmed on a pull 
and build of the latest level of the code (02/22/2000).

Comment 2

[Thomas Lendacky]

After doing some additional investigation I've also found another problem with 
this interface.  It turns out that whenever class nsObserverReleaser: public 
nsDequeFunctor is invoked it performs an NS_RELEASE against each queue observer 
entry.  However, only one NS_ADDREF was performed for the observer during the 
CObserverService::RegisterObservers function.  If the loop to call 
Observer->GetTagNameAt is only executed once this is OK (Observer is only 
interested in one tag).  However, if the Observer->GetTagNameAt wants to return 
multiple tags of interest and gets called multiple times, then problems arise 
because the mRefCnt goes to zero during the nsObserverReleaser: public 
nsDequeFunctor when it shouldn't.  An easy way to check this is to modify 
mozilla\intl\chardet\src\nsMetaCharsetObserver.cpp to return two or more tags of 
interest (it just ignores them during Notify processing).

Comment 3

[buster]

I'll need this as well for editor frameset detection.

Comment 4

[rickg]

This is a good suggestion, and I've implemented it in my tree. It will land 
shortly.

Comment 5

[rickg]

Landed tonight as changed to CDTDUtils.cpp.

Comment 6

[Thomas Lendacky]

I tested the fix and it resolves the multiple calls for a particular tag.  
However, it does not invoke the "Notify" routine for an XML tag.  The "Notify" 
routine interface should probably be modified to pass in an indication of the 
language or language namespace so that HTML and XML can be discerned and tags 
that exist in multiple namespaces can be discerned (this would obviously be a 
change to the ElementObserver interface).

Also, the problem with the class nsObserverReleaser: public 
nsDequeFunctor still exists.  I modified 
mozilla\intl\chardet\src\nsMetaCharsetObserver.cpp to return two tags instead of 
one and after the first invocation of the "Notify" routines it is never called 
again (I believe this is because of the nsSupportsWeakReference).  If I add an 
NS_ADDREF( this ) to the "GetTagNameAt" routine, then the ElementObserver 
interface functions correctly (for HTML).  The NS_RELEASE in the nsDequeFunctor 
should not be done.

Comment 7

[rickg]

Tom: the notify protocol is based on a topic; current text/html and text/xml. 
The parsers use the mimetype in their notification process. If you're 
registerting the _same_ observer for both topics, you won't be able to 
differentiate. But we expect folks to use a seperate observer. 

I'll update the code to deal with the addref/release problem.

Comment 8

[rickg]

I've fixed the addref/release problem. We were not properly addref'ing 
observers.

Comment 9

[Thomas Lendacky]

Ok, that's not a problem to have two separate observers to differentiate between 
HTML and XML.  Tested your fixes and the ADDREF/RELEASE problem is fixed.

However, XML tags are not driving the observer.  The HTML tags are driven from 
CNavDTD.cpp calling into nsDTDUtils.cpp. The XML tags should be driven from 
nsWellFormedDTD.cpp calling into nsDTDUtils.cpp, but this is not occurring (or 
coded).  Also, for the XML Notify function, is there a way to determine the XML 
namespace for the tag/element that the observer is being notified about?  If the 
same tag, for example "XYZ", is defined in two different XML namespaces I can 
see components/observers wanting to process the "XYZ" tag only if it is part of 
a particular namespace for which that component/observer has provided support.

Comment 10

[rickg]

I'm handing the XML observer over to nisheeth for completion. Nisheeth: You 
should be able to copy the code from CNavDTD.

Comment 11

[Nisheeth Ranjan]

I am concerned about the performance implications of this.  In the HTML world, 
we use atoms for doing string comparisons of tags.  In XML, we will probably 
need to do a strcmp() on each incoming tag to determine if we need to notify an 
observer.  What do you guys think?

Tom, please provide a little background around why you need to observe XML 
elements.  Maybe there's a less costly way to get you what you want.

Comment 12

[Thomas Lendacky]

I'm working on implementing P3P within Mozilla.  P3P relies on being able to 
specify information within a "META" tag (soon to be changed to a "LINK" tag) 
within the HTML.  So, I'm relying on the ElementObserver interface to be 
notified of these tags and process them accordingly without having to insert 
P3P related code into the content sink(s).  Since you can have an XML version of 
HTML, I need to observe XML tags also (which is why the interface should have 
the XML namespace associated with the tag as an argument also).

Comment 13

[Nisheeth Ranjan]

Scheduling for M16...

Comment 14

[Nisheeth Ranjan]

Harish, thanks for volunteering to own this.  Re-assigning this to you...

Comment 15

[harishd]

nominating for nsbeta2

Comment 16

[leger]

Putting on [nsbeta2-] radar. 

Comment 17

[harishd]

Tom, I'll provide a patch to unblock you. Will that work?

Moving to M17.

Comment 18

[Thomas Lendacky]

Don't worry about a patch, i'm not blocked right now because I still have the 
HTML path with which I can verify everything I'm doing.  I will eventually need 
to be able to recognize HTML tags within XML but for now I can work around it.

Comment 19

[harishd]

Adding rtm to the keyword.

Comment 20

[harishd]

changing rtm to nsbeta3.

Comment 21

[harishd]

This bug has been marked "future" because we have determined that it is not 
critical for netscape6.0. If you feel this is an error, or if it blocks your 
work in some way -- please attach your concern to the bug for reconsideration.

Comment 22

[Nisheeth Ranjan]

Removed nsbeta3 nomination from futured bugs.

Comment 23

[Thomas Lendacky]

I'm ok with you moving this out.  This will only cause problems for me when HTML 
is XMLized.  Should any XHTML/HXML be encountered that is using the LINK tag to 
indicate the use of P3P, then the privacy support won't be active.

Comment 24

[bsharma]

updated qa contact.

Comment 25

[Boris Zbarsky [:bzbarsky]]

The observer service stuff is gone (see bug 96364).

Further, the tag observer stuff relies on parser nodes and XML parsing hasn't
used those in a good long while.

So tag observers as they currently exist are an HTML-only beast.  Are they
something we want to hack in for XML?

Comment 26

[Johnny Stenback (:jst)]

f*&$k no! :-) If we want something like that, lets set up a SAX chain system
with the sinks being just one link in the chain (most of the time the last and
only one). Tag observers should die, but that won't happen, but let's not make
them spread farther.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Right.  Marking wontfix.