83612 - Correct/cleanup style loading code in content

Status: VERIFIED FIXED

(Core :: XML, defect, P1)

Description

[rbs]

Compilation gives a warning that a 'namespaceID' variable is being used without 
being initialized on line 1098 in nsXMLContentSink.cpp. Not sure how serious 
this is. But I thought I will file this bug since namespace issues are 
sensitive.

nsXMLContentSink.cpp(1098) : warning C4700: local variable 'namespaceID' used 
without having been initialized

Comment 1

[Peter Van der Beken [:peterv]]

The relevant part of my patch for bug 7515 (which changed this code) is this:

   nsAutoString title; 
   nsAutoString type; 
   nsAutoString media; 
 
+  nsCOMPtr<nsIAtom> name, prefix;
+  PRInt32 namespaceID;
+
+  if (NS_FAILED(mStyleElement->GetAttribute(namespaceID,
*getter_AddRefs(name),src)))
+    return rv;
+  src.StripWhitespace();
+
   for (i = 0; i < count; i++) {
-    PRInt32 namespaceID;
-    nsCOMPtr<nsIAtom> name, prefix;
    
mStyleElement->GetAttributeNameAt(i,namespaceID,*getter_AddRefs(name),*getter_AddRefs(prefix));
-    if (name.get() == nsHTMLAtoms::src) {
-      mStyleElement->GetAttribute(namespaceID,name,src);
-      src.StripWhitespace();
-    }
-    else if (name.get() == nsHTMLAtoms::title) {
+    if (name.get() == nsHTMLAtoms::title) {
       mStyleElement->GetAttribute(namespaceID,name,title);
       title.CompressWhitespace();
     }

I think we should set namespaceID to kNameSpaceID_None, as that is what
GetAttributeNameAt does, and that's what initialized namespaceID in the old code
(see
http://lxr.mozilla.org/seamonkey/source/content/html/content/src/nsGenericHTMLElement.cpp#1866).
I'll attach a patch.

Comment 2

[Peter Van der Beken [:peterv]]

I'll use this bug to do some cleanup and corrections for the style-loading code.
First patch coming up, one or two more will follow.

Comment 3

[Peter Van der Beken [:peterv]]

Attachment: Cleanup, fix <style>...</style> and prevent a leak

Comment 4

[rbs]

*** Bug 83744 has been marked as a duplicate of this bug. ***

Comment 5

[Peter Van der Beken [:peterv]]

Attachment: More cleanup, replaces previous patch

Comment 6

[Peter Van der Beken [:peterv]]

I now always set the mParser (in nsStyleLinkElement) to nsnull when setting
mDontLoadStyle to PR_TRUE. This ensures that we don't leak the parser when not
loading stylesheets. Looking for an r and sr.

Comment 7

[Peter Van der Beken [:peterv]]

Attachment: With changes for jst's comments

Comment 8

[Peter Van der Beken [:peterv]]

Attachment: And one more variable gone

Comment 9

[Johnny Stenback (:jst)]

sr=jst

Comment 10

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Um, near the end of the last patch we bail out if there is a src attribute. I
think we should just ignore the src attribute and not bail out if it exists (so
you can also avoid GetAttribute for src). This is of course only for XHTML.

Comment 11

[Peter Van der Beken [:peterv]]

We could do that, it does make some sense. Note that we didn't support that
before. Is the patch ok otherwise?

Comment 12

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Yeah, r=heikki.

Comment 13

[Peter Van der Beken [:peterv]]

Attachment: Ignore src attribute

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Have you checked that there aren't any regressions in
http://www.bath.ac.uk/~py8ieh/internet/importtest/ ?

Comment 15

[Asa Dotzler [:asa]]

a= [e-mail] for checkin to the trunk.
(on behalf of drivers)

Comment 16

[Peter Van der Beken [:peterv]]

Checked in.

Comment 17

[Chris Petersen]

Marking verified per last comments.