Closed Bug 153755 Opened 22 years ago Closed 21 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect, P2)

defect

Tracking

()

VERIFIED FIXED
mozilla1.7alpha

People

(Reporter: bugmail, Assigned: bzbarsky)

References

(Blocks 1 open bug, )

Details

(Keywords: css2, testcase)

Attachments

(2 files, 2 obsolete files)

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.)
Blocks: 153699
Keywords: css2
Attached file 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.
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?
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.
Attached patch Patch (obsolete) — Splinter 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]
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+
Attached file Patch updated to comments (obsolete) —
Attachment #138470 - Attachment is obsolete: true
Attachment #138730 - Attachment is patch: false
Attachment #138730 - Attachment is obsolete: true
Checked in.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Verified using FF 1.5.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: