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)
Core
CSS Parsing and Computation
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)
182 bytes,
text/html
|
Details | |
28.04 KB,
patch
|
Details | Diff | Splinter Review |
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.)
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.
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
Assignee | ||
Comment 4•22 years ago
|
||
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"
Assignee | ||
Comment 6•21 years ago
|
||
I should just take this to keep it on my radar.
Assignee: dbaron → bz-vacation
Priority: -- → P3
Target Milestone: --- → mozilla1.6beta
Assignee | ||
Comment 7•21 years ago
|
||
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.
Assignee | ||
Comment 8•21 years ago
|
||
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.
Assignee | ||
Comment 9•21 years ago
|
||
Comment on attachment 138470 [details] [diff] [review] Patch David, what do you think?
Attachment #138470 -
Flags: superreview?(dbaron)
Attachment #138470 -
Flags: review?(dbaron)
Assignee | ||
Updated•21 years ago
|
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+
Assignee | ||
Comment 12•21 years ago
|
||
Attachment #138470 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Attachment #138730 -
Attachment is patch: false
Assignee | ||
Comment 13•21 years ago
|
||
Attachment #138730 -
Attachment is obsolete: true
Assignee | ||
Comment 14•21 years ago
|
||
Checked in.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•