[FIX]@import rule media type "screen_test" misinterpreted as "screen"

VERIFIED FIXED in mozilla1.7alpha



CSS Parsing and Computation
16 years ago
13 years ago


(Reporter: Greg K., Assigned: bz)


(Blocks: 1 bug, {css2, testcase})

css2, testcase

Firefox Tracking Flags

(Not tracked)




(2 attachments, 2 obsolete attachments)



16 years ago
If an @import media rule includes an unrecognized media type such as
"screen_test", the @import rule is still processed by Mozilla even though it
should be ignored. (See item 5 at the URL provided.)


16 years ago
Blocks: 153699


16 years ago
Keywords: css2
Created attachment 88874 [details]
simple testcase

Note that this seems to be somewhat specific to '_' (or, more likely,
characters that aren't part of identifiers, i.e., a mistake in error handling).
Actually, not even that, since we do the right thing if I replace the '_' with
a ' ' too.


16 years ago
Keywords: testcase
This is because EnumerateMediaString in nsCSSLoader.cpp differs from GatherMedia
in nsCSSParser.cpp (and they duplicate code that should only be in one place). 
We should just use a supports array from GatherMedia and not duplicate the parsing.
Ever confirmed: true
Hmm... the media string could also be coming from

<link media="screen_test" src="foo.css" rel="stylesheet">

or something like that, and there is no reason the "_" is invalid there, is
there?   In particular, would

@import url(foo.css) screen\_attribute;

be valid?

Comment 5

16 years ago
Also reproduced using Win32/2003010408. Setting All/All.
OS: MacOS X → All
Hardware: Macintosh → All
Summary: @import media rule "screen_test" misinterpreted as "screen" → @import rule media type "screen_test" misinterpreted as "screen"
I should just take this to keep it on my radar.
Assignee: dbaron → bz-vacation
Priority: -- → P3
Target Milestone: --- → mozilla1.6beta
Actually, EnumerateMediaString implements the algorithm of
http://www.w3.org/TR/html401/types.html#type-media-descriptors (so we need to
keep it).  Patch coming up to not redo the CSS parser's work, however.
Created attachment 138470 [details] [diff] [review]

I'd like to move to nsCOMArray here at some point, maybe, but I need to resolve
the mess that is the import rule impl first....  I suspect what I _really_ want
to do is to simply guarantee that import rules always have sheets, then kill
off all the state the import rule stores and make it fetch it off the sheet.

The ReleaseScanner() calls I added are not directly related to this bug, but
are needeed in any case (we don't properly release the scanner in those error
cases), so I figured I wouldn't remove them from the patch.
Comment on attachment 138470 [details] [diff] [review]

David, what do you think?
Attachment #138470 - Flags: superreview?(dbaron)
Attachment #138470 - Flags: review?(dbaron)
Priority: P3 → P2
Summary: @import rule media type "screen_test" misinterpreted as "screen" → [FIX]@import rule media type "screen_test" misinterpreted as "screen"
Target Milestone: mozilla1.6beta → mozilla1.7alpha
Comment on attachment 138470 [details] [diff] [review]

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

> // Parse a CSS2 media rule: "@media medium [, medium] { ... }"
> PRBool CSSParserImpl::ParseMediaRule(nsresult& aErrorCode, RuleAppendFunc aAppendFunc,
>                                      void* aData)
> {
>   nsAutoString  mediaStr;
>   nsCOMPtr<nsISupportsArray> media;
>-  NS_NewISupportsArray(getter_AddRefs(media));
>+  aErrorCode = NS_NewISupportsArray(getter_AddRefs(media));
>   if (media) {
>-    if (GatherMedia(aErrorCode, mediaStr, media)) {
>-      if ((!mediaStr.IsEmpty()) &&
>+    if (GatherMedia(aErrorCode, media)) {
>+      // XXXbz this could use better error reporting throughout the method
>+      PRUint32 count;
>+      media->Count(&count);
>+      if (count > 0 &&
>           ExpectSymbol(aErrorCode, '{', PR_TRUE)) {
>         // push media rule on stack, loop over children
>         nsCOMPtr<nsICSSMediaRule>  rule;

Remove the |mediaStr| variable.

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

>+CSSImportRuleImpl::CSSImportRuleImpl(nsISupportsArray* aMedia)
>   : nsCSSRule(),
>     mURLSpec()
> {
>-  NS_NewMediaList(getter_AddRefs(mMedia));
>+  // XXXbz This is really silly.... the mMedia we create here will be
>+  // clobbered if we manage to load a sheet.  Which should really
>+  // never fail nowadays, in sane cases.
>+  NS_NewMediaList(aMedia, nsnull, getter_AddRefs(mMedia));
> }
> CSSImportRuleImpl::CSSImportRuleImpl(const CSSImportRuleImpl& aCopy)
>   : nsCSSRule(aCopy),
>     mURLSpec(aCopy.mURLSpec)
> {
>+  nsCOMPtr<nsICSSStyleSheet> sheet;
>   if (aCopy.mChildSheet) {
>     aCopy.mChildSheet->Clone(nsnull, this, nsnull, nsnull,
>-                             getter_AddRefs(mChildSheet));
>-  }
>-  NS_NewMediaList(getter_AddRefs(mMedia));
>-  if (aCopy.mMedia && mMedia) {
>-    mMedia->AppendElement(aCopy.mMedia);
>+                             getter_AddRefs(sheet));
>   }
>+  SetSheet(sheet);

It looks like you can remove the 1 and 2 parameter forms of NS_NewMediaList
because of these changes..
Comment on attachment 138470 [details] [diff] [review]

>Index: content/html/style/src/nsCSSLoader.h
> #include "nsDataHashtable.h"
>+#include "nsISupportsArray.h"

Why?  You only need, here, the forward declaration that you already have from
nsICSSLoader.h.  I'd think it would be better to include only where used.

r+sr=dbaron with the changes in this comment and the previous one.
Attachment #138470 - Flags: superreview?(dbaron)
Attachment #138470 - Flags: superreview+
Attachment #138470 - Flags: review?(dbaron)
Attachment #138470 - Flags: review+
Created attachment 138730 [details]
Patch updated to comments
Attachment #138470 - Attachment is obsolete: true
Attachment #138730 - Attachment is patch: false
Created attachment 138732 [details] [diff] [review]
Er, for real now.  ;)
Attachment #138730 - Attachment is obsolete: true
Checked in.
Last Resolved: 15 years ago
Resolution: --- → FIXED

Comment 15

13 years ago
Verified using FF 1.5.
You need to log in before you can comment on or make changes to this bug.