93977 - CSSMediaListImpl should implement nsIDOMCSSMediaRule

Status: RESOLVED FIXED

(Core :: DOM: CSS Object Model, defect)

Description

[Boris Zbarsky [:bzbarsky]]

CSSMediaListImpl should implement nsIDOMCSSMediaRule to comply with the DOM2
Style specification

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Preliminary patch

Comment 2

[Boris Zbarsky [:bzbarsky]]

In addition to implementing nsIDOMCSSMediaRule, this patch fixes the medialist
interface to have the right function names, cleans up some memory leaks by
switching to nsCOMPtrs for things pulled out of nsISupportsArrays, and fixes the
implementation of InsertRule() on CSSStyleSheetImpl.

The patch is pretty rough.  Things that still need fixing:

1)  CSSStyleSheetImpl::DeleteRule should be throwing a DOM_INDEX_SIZE_ERR when
    the index is out of range
2)  The rule insertion functions should throw hierarchy errors as appropriate
3)  I overuse NS_ENSURE_SUCCESS -- some of those uses should not be asserting.

Apart from those, this patch is fully functional.  attaching amusing testcase so
people can play with it a bit.
Any comments on the code would be very much appreciated.  If this is totally the
wrong approach to take, it'd be nice to know now instead of after I've sunk in
tons of time polishing the patch.  :)

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: testcase

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Cleaned up patch

Comment 5

[Boris Zbarsky [:bzbarsky]]

OK.  This was annoying me and I was bored, so I cleaned it up some anyway.  Reviews?

Comment 6

[Johnny Stenback (:jst)]

A few nits about the patch:

- No need to assign nsnull into mMedia since getter_AddRefs(mMedia) will zero
out mMedia (this occures in a few places).

+    mMedia->DropReference();
+    mMedia = nsnull;
+    rv = NS_NewMediaList(getter_AddRefs(mMedia), oldMedia, aSheet);

- The out parameter in NS_NewMediaList() should really be the last parameter and
not the first one, I know we violate this in lots of places and maby it makes
sense to leave it as it is here for consistency, change it if you like to.

- Eeeew, could we not do this:

+      return NS_COMFALSE;

If it's avoidable, please don't do that, don't overload the return value from
XPCOM methods (I know you didn't introduce this, but now would be a good time to
clean this up if you're up to it). Even declaring the method as
NS_IMETHOD_(PRBool) (or even just PRBool if it's not part of an interface) would
be cleaner than what we have now, IMO.
- Could mNameSpace be made a nsCOMPtr here?

+      NS_RELEASE(mInner->mNameSpace);
+      mInner->mNameSpace = newNameSpace; // takes ref
- Code liks this:

+  PRInt32 type;
+  cssRule->GetType(type);
+  if (type != nsICSSRule::STYLE_RULE) {
+    return NS_ERROR_DOM_HIERARCHY_REQUEST_ERR;
+  }
really should initialize 'type' to something in case the call to GetType()
either fails or ends up calling a method that's not implemented and doesn't set
the out parameter.

Someone more knowledgeble in the style system code should have a close look at
this patch, but I'm willing to give my sr=jst here assuming this is well tested
n' all...

sr=jst

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch updated to address jst's comments

Comment 8

[Boris Zbarsky [:bzbarsky]]

Pierre, could you review this?  Is there anyone else who should take a look at this?

Ian, know any good testcases for this stuff?  I wrote one up, as you see, but I
may have missed some aspects of the spec...

I've been browsing with this for a few days and running things like
browserbuster and what not, and it seems to not be doing anything evil. :)

Comment 9

[Pierre Saslawsky]

hyatt & dbaron should take a look too, otherwise I'll review in the next couple 
of days.

Comment 10

[Hixie (not reading bugmail)]

bz: Nothing substantial, but check if you see anything at:
   http://www.hixie.ch/tests/adhoc/dom/css/
   http://www.hixie.ch/tests/ngdriver/domcss/
Sorry, DOM2 tests are not a priority for me right now to be honest...

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch that includes a fix for bug 95902 (memory leak / compile warning fix)

Comment 12

[Boris Zbarsky [:bzbarsky]]

So.... reviews?

Comment 13

[Johnny Stenback (:jst)]

One more nit, I see inconsistensies with if/else formatting:

  if (...) {
    ...
  } else {
    ...
  }

and:

  if (...) {
    ...
  }
  else {
    ...
  }

Stick to what's more common in the files.

- In CSSMediaRuleImpl::InsertStyleRuleAt() there's an else-after-return:

+  if (mRules->InsertElementAt(aRule, aIndex)) {
+    aRule->SetStyleSheet(mSheet);
+    return NS_OK;
+  } else {
+    return NS_ERROR_FAILURE;
+  }
+}

skip the 'else', and you might wanto turn that logic around so that the error
case does the early return and the success case runs to the end of the method.

Fix those nits when checking in, sr=jst

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 47901 [details] [diff] [review]
Patch that includes a fix for bug 95902 (memory leak / compile warning fix)

Fixed jst's nits.  Pierre?  hyatt?  dbaron?

Comment 15

[Pierre Saslawsky]

Comment on attachment 47901 [details] [diff] [review]
Patch that includes a fix for bug 95902 (memory leak / compile warning fix)

r=pierre

Comment 16

[Boris Zbarsky [:bzbarsky]]

fix checked into trunk.

Comment 17

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

A few belated comments:

nsCSSStyleSheet.cpp:

 * (also in nsCSSRules.cpp) why the changes from |ElementAt| to
|QueryElementAt|?  If you know you put in an nsIAtom, you'll get an nsIAtom back
out.  There's no need to waste time doing a QueryInterface to get back what you
already have.

 * Why add the extra |mRuleCascades = nsnull;| when there already is one?

 * Why change |RebuildNameSpaces| to use a dummy pointer to mNameSpace to pass
to |CreateNameSpace| rather than the real one -- can't this lose track of, and
leak, what's added?  Don't you really mean |address_of| (since it's in-out, and
you changed it to an nsCOMPtr)?

nsCSSParser.cpp:
 * The optionally-null |aResult| seems a bit odd -- why not have it always pass
the result out to the caller, and have the caller do the |AppendRule| in the
normal case?

 * The |OUTPUT_ERROR();| seems misplaced -- there's no error to output since
nothing's been parsed yet.  Shouldn't it be right before |ReleaseScanner|?
 

Also, something in this checkin is also leaking CSS rules (as shown by tinderbox
leak stats).

Comment 18

[Pierre Saslawsky]

Reopening per dbaron's comments in order to fix the memory leaks.  It would be a 
good idea to look at the other things at the same time.

Comment 19

[Boris Zbarsky [:bzbarsky]]

> A few belated comments:

Sorry about checking this in, then... Did not realize you were looking at it.

> why the changes from |ElementAt| to |QueryElementAt|?

There was a number of places in which |ElementAt| was being misused and things
were not released properly....  |QueryElementAt| more obviously addrefs.  There
were also a few places that _did_ require a queryinterface (though generally
they had that in the code already).  The switch was mostly to make memory issues
clearer, but if there is a significant cost associated with QIing I will
certainly change as many of the instances as I can to using typecasts and
assignments to nsCOMPtr's using |dont_AddRef|

> Why add the extra |mRuleCascades = nsnull;| when there already is one?

Because I suck and did not properly do the merge when you fixed that...

> Don't you really mean |address_of|

I think I might.  If I have |nsCOMPtr<nsIFoo> foo| and I pass |address_of(foo)|
to a function that then tries to cast it to |nsIFoo**|, will the cast do the
right thing?

> The optionally-null |aResult| seems a bit odd -- why not have it always pass
> the result out to the caller, and have the caller do the |AppendRule| in the
> normal case?

That seems like a much better approach.  Will do that.

> The |OUTPUT_ERROR();| seems misplaced -- there's no error to output

Hmm.  Indeed.  I need to actually add a REPORT_UNEXPECTED before that...

> Also, something in this checkin is also leaking CSS rules

Doh.  So it is.  How do I enable the leak logging so I can debug this?

Comment 20

[Boris Zbarsky [:bzbarsky]]

Nevermind.  Found http://lxr.mozilla.org/seamonkey/source/xpcom/doc/MemoryTools.html

Will hunt for leaks.

Comment 21

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

Also see http://www.mozilla.org/performance/leak-tutorial.html for leak hunting
information.  (Hopefully you either have access to Linux or Mac, or can get the
Windows stack walking code to work for you -- which I couldn't.)

Comment 22

[Boris Zbarsky [:bzbarsky]]

I have linux.... Looks like an nsCSSRule is being leaked by
nsCSSStyleSheet::InsertRule() somehow (attaching log; ParseRule() is a getter,
so I would expect the refcount to be 1 at the end of it).  Looking into this
further.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Attachment: output of make-tree.pl for one of the leaked nsCSSRules

Comment 24

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

nsCSSParser::ParseRuleSet has a while loop, in which you assign to and AddRef
aResult multiple times.  This probably explains why some people were seeing
visited links not underlined after the patch.

Comment 25

[lchiang]

comments in bug http://bugzilla.mozilla.org/show_bug.cgi?id=98358 indicates that
the cause may be this bug. Just a fyi.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch addressing dbaron's comments and fixing regression bug 98358

Comment 27

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

Comment on attachment 48351 [details] [diff] [review]
Patch addressing dbaron's comments and fixing regression bug 98358

>Index: content/html/style/public/nsICSSParser.h
>+// Rule processing function
>+typedef void (*RuleAppendFunc) (nsICSSRule* aRule, void* aData);

Should be
  typedef void (*PR_CALLBACK RuleAppendFunc) (nsICSSRule* aRule, void* aData);
(PR_CALLBACK added).

>Index: content/html/style/src/nsCSSParser.cpp

>+static void AppendRuleToArray(nsICSSRule* aRule, void* aArray)
>+static void AppendRuleToSheet(nsICSSRule* aRule, void* aParser)

Replace |static void| with |PR_STATIC_CALLBACK(void)| in both of these.

>   nsCSSToken* tk = &mToken;
>   // Get first non-whitespace token
>   if (!GetToken(errorCode, PR_TRUE)) {
>+    REPORT_UNEXPECTED(
>+      NS_LITERAL_STRING("Whitespace-only string given to be parsed as rule."));
>     OUTPUT_ERROR();
>   } else if (eCSSToken_AtKeyword == tk->mType) {
>-    ParseAtRule(errorCode, aResult);    
>+    ParseAtRule(errorCode, AppendRuleToArray, *aResult);    
>   }
>   else {
>     UngetToken();
>-    ParseRuleSet(errorCode, aResult);
>+    ParseRuleSet(errorCode, AppendRuleToArray, *aResult);
>   }

I think you should also have an |OUTPUT_ERROR();| right here.

>   ReleaseScanner();
>   return NS_OK;


>@@ -1004,20 +1025,12 @@
...
>-
>+  (*aAppendFunc)(rule, aData);
>+  

No need to add whitespace on blank lines.

(nsCSSRules.cpp)
>     return NS_ERROR_FAILURE;
>   }
>-  aRule->SetStyleSheet(mSheet);
>   return NS_OK;

Why did you remove the |SetStyleSheet|?


>Index: content/html/style/src/nsCSSStyleSheet.cpp
>@@ -2717,95 +2714,126 @@

Do you really need to expand the rule-type checking stuff so much?
Isn't the only time you get multiple rules back is when you had a
single ruleset with grouped selectors?

Comment 28

[Boris Zbarsky [:bzbarsky]]

added PR_CALLBACK stuff and OUTPUT_ERROR before |ReleaseScanner()|

> Why did you remove the |SetStyleSheet|?

I actually have:

-  // There is no xpcom-compatible version of InsertElementAt.... :(
-  if (! mRules->InsertElementAt(aRule, aIndex)) {
+  aRules->EnumerateForwards(SetStyleSheetReference, mSheet);
+  // There is no xpcom-compatible version of InsertElementsAt.... :(
+  if (! mRules->InsertElementsAt(aRules, aIndex)) {
     return NS_ERROR_FAILURE;
   }
-  aRule->SetStyleSheet(mSheet);

So what I did was replace |aRule->SetStyleSheet(mSheet);| with
|aRules->EnumerateForwards(SetStyleSheetReference, mSheet);| (since I now have
an array of rules instead of a single rule).  I also changed the order of
stylesheet setting and insertion to make it easier to enumerate only over the
new rules.
> Isn't the only time you get multiple rules back is when you had a
> single ruleset with grouped selectors?

At the moment, yes.  If you'd like me to assume this in that part of the code, I
could do that.  I'd sort of like to shorten the rule-type checking as much as
possible myself.... What I have seems somewhat safer against the vagaries of
internal parser changes....

Comment 29

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch addressing second set of dbaron's comments

Comment 30

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

Comment on attachment 48398 [details] [diff] [review]
Patch addressing second set of dbaron's comments

r=dbaron

Comment 31

[Johnny Stenback (:jst)]

CSSParserImpl::ProcessImport() could use some more error checking,
NS_NewCSSImportRule() can fail (out of memory, if nothing else).

Other than that, sr=jst

Comment 32

[Boris Zbarsky [:bzbarsky]]

added a return value check to the NS_NewCSSImportRule() call, and checked in