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

VERIFIED FIXED in mozilla1.7alpha

Status

()

Core
CSS Parsing and Computation
P2
normal
VERIFIED FIXED
16 years ago
13 years ago

People

(Reporter: Greg K., Assigned: bz)

Tracking

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

Trunk
mozilla1.7alpha
css2, testcase
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

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.)
(Reporter)

Updated

16 years ago
Blocks: 153699
(Reporter)

Updated

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.
(Reporter)

Updated

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.
Status: UNCONFIRMED → NEW
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?
(Reporter)

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]
Patch

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]
Patch

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]
Patch

>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(void)
>+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]
Patch

>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.
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
(Reporter)

Comment 15

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