81989 - STYLE tags can cause multiple copies of stylesheet to be loaded

Status: VERIFIED FIXED

(Core :: Layout, defect, P1)

Description

[Peter Van der Beken [:peterv]]

I was going to keep this clean-up to the HTML content sink for later, but I've
discovered that without it we load multiple copies of stylesheets specified
through a <style src=...> tag. This is very bad, I think we should try to get
this in before tree closure.

Comment 1

[Peter Van der Beken [:peterv]]

Attachment: Diff -w -u

Comment 2

[Peter Van der Beken [:peterv]]

Attachment: Same as previous but diff -u

Comment 3

[Johnny Stenback (:jst)]

The changes look good to me, one additional change that could be made here:

       for (i = 0; i < count; i++) {
         nsAutoString key(aNode.GetKeyAt(i));
-        if (key.EqualsIgnoreCase("src")) {
-          GetAttributeValueAt(aNode, i, src);
-          src.StripWhitespace();
-        }
-        else if (key.EqualsIgnoreCase("title")) {
+        if (key.EqualsIgnoreCase("title")) {

is to change the 'key' variable to be a const nsAReadableString& here which
would avoid a string copy per attribute, if this change is made the the
EqualsIgnoreCase() calls need to be changed to Compare() calls to get case
insensitive compare.

Either way, sr=jst

Comment 4

[Johnny Stenback (:jst)]

Or even better, use element->GetAttribute() to get the value of all those
attributes as you do for getting the src attribute. But again, that's not
required...

Comment 5

[Peter Van der Beken [:peterv]]

Attachment: With jst's suggestion

Comment 6

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

This looks ok to me too, r=heikki.

Comment 7

[Johnny Stenback (:jst)]

My sr=jst still applies...

Comment 8

[Peter Van der Beken [:peterv]]

I think this should go in for 0.9.1. I have a fix and it is low-risk. Trying to
load the same styleheet multiple times (though it is only specified once) is a
performance hit, and the document ends up with a style model which is wrong.

Comment 9

[Brendan Eich [:brendan]]

Uhhhh...

-  if(mCurrentContext!=nsnull) {
+  if (!mCurrentContext) {
     parent=mCurrentContext->mStack[mCurrentContext->mStackPos-1].mContent;
   }

Same again, later on.  Use 'if (p)' rather than 'if (p != nsnull)', and 'if
(!p)' rather than 'if (p == nsnull)' -- and try spaces around operators, my eyes
thank you!

/be

Comment 10

[Brendan Eich [:brendan]]

Was the patch really tested?  Need a new patch, review and super-review before
we can go for approval in 0.9.1.

/be

Comment 11

[Peter Van der Beken [:peterv]]

Attachment: Fix my stupidness

Comment 12

[Peter Van der Beken [:peterv]]

Fixed the gross errors. Fixed what I think Brendan meant with "try spaces around
operators" (in one line). Tested with the actual patch I attached, only one
stylesheet is loaded and shows up when doing "dump stylesheets". Re-applying for
r and sr.

Comment 13

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

Once again, r=heikki.

I seem to have a habit of thinking some of those boolean logic changes are wrong
when they are correct, and correct when they are wrong :/ All bow to Brendan :)

Comment 14

[Johnny Stenback (:jst)]

Oh geez, I can't believe I didn't see that, now I'm embarrassed. Thanks for
catching that, Brendan.

Comment 15

[Christopher Blizzard (:blizzard)]

+          nsIDOMText* tc;
+          rv = text->QueryInterface(NS_GET_IID(nsIDOMText), (void**)&tc);
[...]
+            tc->SetData(content);
+            NS_RELEASE(tc);

any reason why you aren't using nsCOMPtr<> there?  Same with the nsIContent
*text above and below.

other than that, sr=blizzard

Comment 16

[Christopher Blizzard (:blizzard)]

oh, and a=blizzard for 0.9.1 when you respond to my one comment.

Comment 17

[Peter Van der Beken [:peterv]]

Attachment: With two conversions to nsCOMPtr

Comment 18

[Peter Van der Beken [:peterv]]

Attachment: Same as 36192 but diff -u -w

Comment 19

[Chris Waterson]

Is this fixed now, peterv? Looks like it went in last night...

Comment 20

[Chris Waterson]

Note that the variable `i' is now unused...

c:\seamonkey\mozilla\content\html\document\src\nsHTMLContentSink.cpp(4663) :
warning C4101: 'i' : unreferenced local variable

Comment 21

[Thomas Swan]

Memory leaks went up pretty steep after this fix went in... 

Comment 22

[Peter Van der Beken [:peterv]]

Bug 83093 deals with the leaks.

Comment 23

[Hixie (not reading bugmail)]

This is actually an HTML element bug, not a CSS bug (the CSS part was working
fine, it was just being called twice). Sending to HTML Element for verification.

Comment 24

[bsharma]

Verified on:
build: 2001-07-02-04-Trunk
platform: WinNT

Below is the test case that I used to verify this bug. 
The test case loads fine.
<HTML>
<body>
<P STYLE="color: red; font-family: 'New Century Schoolbook', serif src:..."> 
This paragraph is styled in red with the New Century Schoolbook font, 
if available.</P>
</BODY>
</HTML>

Comment 25

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

SPAM. HTML Element component deprecated, changing component to Layout. See bug
88132 for details.