<a class="header-button" href="https://bugzilla.mozilla.org/home" title="Go to home page"> Bugzilla

Comment 3

•

12 years ago

Bug 727834 should fix this as well. Will need to add tests for this specific case here.

Depends on: 727834

Comment 4

•

12 years ago

Hmm, no. What needs to change is how we load the source (using the charset as declared) and we we need to save it back into this particular charset rather than to UTF-8.

No longer depends on: 727834

Daniel Glazman (:glazou) (not active in Mozilla any more)

Comment 6

•

12 years ago

So, being able to know which charset to use is not really straightforward... irc brainsform for reference: <cedricv> glazou: you can get this.styleSheet from there <glazou> cool then I can fix it let me update my firefox tree and I'll submit a patch <cedricv> cool :) <glazou> smontagu, cedric, espadrine: http://twitpic.com/9r7lum <glazou> ok, so I can decode correctly based on a @charset rule, but that's not the only way to set a stylesheet's encoding... Cf. http://www.w3.org/TR/CSS2/syndata.html#charset <glazou> and AFAIK, I can't have easy access to that information from StyleEditor.jsm <smontagu> glazou: that's what I've been trying to say for some time :-P <glazou> smontagu: sigh ; let me make a fix that works at least for @charset we clearly miss here an extension to DOMCSSStyleSheet giving the real encoding of the sheet <cedricv> glazou: this.styleSheet.ownerNode.charset ? <glazou> not enough <cedricv> yes, for point 3, then have to dig the info from ownerDocument if not specified there <glazou> if sheet's encoding comes from HTTP, then that holds the empty string again, that's not enough <cedricv> I believe point 1 (HTTP) is handled by NetUtil? <smontagu> cedricv: apparently not. The case where I originally encountered the bug, http://ar.wikipedia.org has a Content-Type header <smontagu> ("Why not?" would be a good question here IMHO) <cedricv> smontagu: right, but the underlying nsIChannel should give us contentCharset :) <smontagu> i.e. why doesn't NetUtils.readInputStreamToString automatically use that contentCharset?) <cedricv> smontagu: good question, i thought it did ;( maybe it just cannot get the underlying channel from the stream (which is what readInputStream gets as argument) if it's that low-level, being in NetUtils is a bit confusing imho <m_kato_> smontagu, cedricv: charset param is to reading file input stream as specific charset. this param is added on firefox 10. <glazou> building <m_kato_> I didn't consider from http input stream. <glazou> honestly we should not have to dive that level deep to get the specified charset for a stylesheet that's a design bug in the css om

Comment 7

•

12 years ago

Attached patch WIP (obsolete) — Details — Splinter Review

Daniel Glazman (:glazou) (not active in Mozilla any more)

Comment 8

•

12 years ago

Attached patch WIP #2 (obsolete) — Details — Splinter Review

Work in progress #2. Successfully tested on: 1. test case exactly as described by reporter 2. http://ar.wikipedia.org (see stylesheet with 358 rules, it has classes in arabic) 3. http://tbpl.mozilla.org/ that has one stylesheet in UTF-8 with no HTTP nor @charset setting. See content properties.

Attachment #628628 - Attachment is obsolete: true

Updated

•

12 years ago

Attachment #628630 - Attachment is patch: true

Comment 9

•

12 years ago

Comment on attachment 628630 [details] [diff] [review] WIP #2 Review of attachment 628630 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, just have a few style/typo nits. ::: browser/devtools/styleeditor/StyleEditor.jsm @@ +801,5 @@ > + * source of a CSS stylesheet, loaded from file or cache > + * @param string aChannelCharset > + * charset of the source string if set by the HTTP channel > + */ > + _fixSourceCharset: function SE__decodeIfCharsetAtRule(aString, aChannelCharset) SE__fixSourceCharset on the right-side. Or preferably a more self-explaining name like SE__decodeSourceToUTF8AsNeeded. @@ +808,5 @@ > + // See http://www.w3.org/TR/CSS2/syndata.html#charset > + try { > + let charset = aChannelCharset; // step 1 of syndata.html > + > + // do we have a @charset rule in the stylesheet? If I understand the spec correctly, the HTTP charset has the highest priority, so if present we should not care about @charset at all and decode directly? @@ +814,5 @@ > + let sheet = this.styleSheet; > + if (sheet && sheet.cssRules) { // sanity check > + let rules = sheet.cssRules; > + if (rules.length > + && rules.item(0).type == Components.interfaces.nsIDOMCSSRule.CHARSET_RULE) { Ci. @@ +819,5 @@ > + // yep we do have one > + charset = rules.item(0).encoding; > + } > + } > + I guess we can add step 3 and step 4 with something like: if (!charset) { // step 3 if charset not already known charset = sheet.ownerNode.getAttribute("charset"): } if (!charset) { // step 4 (1 of 2) if charset not already known let parentSheet = sheet.parentStyleSheet; if (parentSheet && parentSheet.cssRules && parentSheet.cssRules[0].type == Ci.nsIDOMCSSRule.CHARSET_RULE) { charset = parentSheet.cssRules[0].encoding; } if (!charset) { // step 4 (2 of 2) if charset not already known charset = sheet.ownerNode.ownerDocument.characterSet; } } @@ +821,5 @@ > + } > + } > + > + // step 5 of syndata.html > + if (!charset) Braces. @@ +825,5 @@ > + if (!charset) > + charset = "UTF-8"; > + > + // decode > + let converter = Components.classes["@mozilla.org/intl/scriptableunicodeconverter"] Cc[ @@ +826,5 @@ > + charset = "UTF-8"; > + > + // decode > + let converter = Components.classes["@mozilla.org/intl/scriptableunicodeconverter"] > + .createInstance(Components.interfaces.nsIScriptableUnicodeConverter); Ci. @@ +868,4 @@ > { > let channel = Services.io.newChannel(aHref, null, null); > let chunks = []; > + let channel_charset = ""; channelCharset @@ +876,5 @@ > } > }.bind(this), > onDataAvailable: function (aRequest, aContext, aStream, aOffset, aCount) { > + let channel = aRequest.QueryInterface(Ci.nsIChannel); > + if (!channel_charset) Braces. @@ +886,4 @@ > return this._signalError(LOAD_ERROR); > } > > + this._onSourceLoad(this._fixSourceCharset(chunks.join(""), channel_charset)); Perhaps we should avoid the 2 call sites and just call _fixSourceCharset (or _decodeSourceToUTF8IfNeeded) from _onSourceLoad (with an added optional charset argument).

Attachment #628630 - Flags: feedback+

Comment 10

•

12 years ago

Also, we probably want to extract the charset-detection logic to its own method instead (_detectCharset) and store this into a new StyleEditor.charset getter so that we can decode in the reverse way when saving.

Assignee

Comment 11

•

12 years ago

Attached patch Fix that implements all the steps of the spec. (obsolete) — Details — Splinter Review

This patch fixes this bug. Try push at https://tbpl.mozilla.org/?tree=Try&rev=70fbba331703. Note that I don't do the BOM check, but it isn't important.

Assignee: nobody → thaddee.tyl

Attachment #628630 - Attachment is obsolete: true

Attachment #653979 - Flags: review?(mihai.sucan)

Comment 12

•

12 years ago

Comment on attachment 653979 [details] [diff] [review] Fix that implements all the steps of the spec. Review of attachment 653979 [details] [diff] [review]: ----------------------------------------------------------------- Patch looks good, thank you Thaddee! Only small suggestions below. Please also add a test for this bug. We need to make sure that we do not cause regressions in the future. Thanks! ::: browser/devtools/styleeditor/StyleEditor.jsm @@ +763,5 @@ > + * > + * @param string aString > + * source of a CSS stylesheet, loaded from file or cache > + * @param string aChannelCharset > + * charset of the source string if set by the HTTP channel nit: start with upper case. @@ +765,5 @@ > + * source of a CSS stylesheet, loaded from file or cache > + * @param string aChannelCharset > + * charset of the source string if set by the HTTP channel > + */ > + _decodeCSSCharset: function SE__decodeCSSCharset(aString, aChannelCharset) This method name sounds weird for me. This method detects the stylesheet charset and takes a string encoded in some charset that we hope that we match... and then it converts the string to utf8. Maybe you want to split this method into two: one that determines the charset to use, and another method which does the actual conversion, if needed. @@ +769,5 @@ > + _decodeCSSCharset: function SE__decodeCSSCharset(aString, aChannelCharset) > + { > + // StyleSheet's charset can be specified from multiple sources > + // See http://www.w3.org/TR/CSS2/syndata.html#charset > + try { I prefer wrapping things in a try-catch where you know the code can throw, instead of wrapping the entire method. If something fails and it's our fault we will never know with a try-catch that simply hides everything. @@ +786,5 @@ > + if (sheet && sheet.cssRules) { // sanity check > + let rules = sheet.cssRules; > + if (rules.length > + && rules.item(0).type == Ci.nsIDOMCSSRule.CHARSET_RULE) { > + // Yep we do have one. Is this comment needed? @@ +793,5 @@ > + } > + } > + > + // step 3: see <link charset="…"> > + let linkCharset = sheet.ownerNode.getAttribute("charset"); I'm confused; in step 2 you do a check to make sure |sheet| has a value and that it has the |cssRules| property. However, here in step 3 you assume |sheet| always has a value and an owner node. Looking through the file it seems that this.styleSheet is always available by the time this code is executed - but please double-check. Please make this function consistent - either always check, or just don't check. Also, can you assume sheet.ownerNode is always available? Looking at the spec, it seems ownerNode can be null for @imported styles. [1] [1] http://www.w3.org/TR/DOM-Level-2-Style/stylesheets.html @@ +837,4 @@ > } > let source = NetUtil.readInputStreamToString(aStream, aStream.available()); > aStream.close(); > + this._onSourceLoad(source, ""); Do we always get an utf8 result from reading a file? Even if it's latin1 or some other encoding? Also, by providing no charset here you allow the _decodeCSSCharset() method to apply heuristics that might not be valid for a file loaded from the disk. @@ +886,5 @@ > * > * @param string aSourceText > + * @param string aCharset > + * The character set to use (don't provide this information if > + * irrelevant). You could change this to: "Optional. The character set to use. The default is to detect the charset from the stylesheet or from the page."

Attachment #653979 - Flags: review?(mihai.sucan)

Assignee

Comment 13

•

12 years ago

(In reply to Mihai Sucan [:msucan] from comment #12) > @@ +837,4 @@ > > } > > let source = NetUtil.readInputStreamToString(aStream, aStream.available()); > > aStream.close(); > > + this._onSourceLoad(source, ""); > > Do we always get an utf8 result from reading a file? Even if it's latin1 or > some other encoding? From reading a file, we always get ISO-Latin-1. That's why we need to convert it to Unicode. > Also, by providing no charset here you allow the _decodeCSSCharset() method > to apply heuristics that might not be valid for a file loaded from the disk. The CSS heuristics still need to be applied (we need to take @charset into account, even for stylesheets on disk).

Assignee

Comment 14

•

12 years ago

Attached patch Fix that implements all the steps of the spec. (obsolete) — Details — Splinter Review

Attachment #653979 - Attachment is obsolete: true

Attachment #655401 - Flags: review?(mihai.sucan)

Comment 15

•

12 years ago

Comment on attachment 655401 [details] [diff] [review] Fix that implements all the steps of the spec. Review of attachment 655401 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for your update! I would give this r+ but I get a test failure: TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/styleeditor/test/browser_styleeditor_init.js | stylesheet is unicode-aware. This always happens for me. ::: browser/devtools/styleeditor/StyleEditor.jsm @@ +766,5 @@ > + * Source of a CSS stylesheet, loaded from file or cache. > + * @param string aChannelCharset > + * Charset of the source string if set by the HTTP channel. > + */ > + _decodeCSSCharset: function SE__decodeCSSCharset(aString, aChannelCharset) Does this method actually use aString? Would it be better to only return the charset name that should be used? Something like: charset = this._determineCSSCharset(theChannelCharsetIHave); string = this._convertToUnicode(string, charset); @@ +804,5 @@ > + } > + > + if (sheet.ownerNode) { > + // step 4 (2 of 2): charset of referring document. > + if (sheet.ownerNode.ownerDocument.characterSet) { Why not roll these two if conditions into one? if (sheet.ownerNode && sheet.ownerNode.ownerDocument.characterset) { ... }

Attachment #655401 - Flags: review?(mihai.sucan)

Updated

•

12 years ago

Status: NEW → ASSIGNED

Assignee

Comment 16

•

12 years ago

(In reply to Mihai Sucan [:msucan] from comment #15) > Comment on attachment 655401 [details] [diff] [review] > Fix that implements all the steps of the spec. > > Review of attachment 655401 [details] [diff] [review]: > ----------------------------------------------------------------- > > Thanks for your update! I would give this r+ but I get a test failure: > > TEST-UNEXPECTED-FAIL | > chrome://mochitests/content/browser/browser/devtools/styleeditor/test/ > browser_styleeditor_init.js | stylesheet is unicode-aware. > > This always happens for me. I cannot reproduce this at all. Every time I run the mochitests, I get complete success. I have started a try push to investigate this, at https://tbpl.mozilla.org/?tree=Try&rev=430a0f7a10a2, but it will require time, unless you are willing to investigate this issue yourself. > ::: browser/devtools/styleeditor/StyleEditor.jsm > @@ +766,5 @@ > > + * Source of a CSS stylesheet, loaded from file or cache. > > + * @param string aChannelCharset > > + * Charset of the source string if set by the HTTP channel. > > + */ > > + _decodeCSSCharset: function SE__decodeCSSCharset(aString, aChannelCharset) > > Does this method actually use aString? Would it be better to only return the > charset name that should be used? > > Something like: > charset = this._determineCSSCharset(theChannelCharsetIHave); > string = this._convertToUnicode(string, charset); I feel against that, because this method doesn't have any use without the call to _convertToUnicode(). > @@ +804,5 @@ > > + } > > + > > + if (sheet.ownerNode) { > > + // step 4 (2 of 2): charset of referring document. > > + if (sheet.ownerNode.ownerDocument.characterSet) { > > Why not roll these two if conditions into one? > > if (sheet.ownerNode && sheet.ownerNode.ownerDocument.characterset) { ... } You're right. It's going in my next patch.

Comment 17

•

12 years ago

(In reply to Thaddee Tyl [:espadrine] from comment #16) > (In reply to Mihai Sucan [:msucan] from comment #15) > > Comment on attachment 655401 [details] [diff] [review] > > Fix that implements all the steps of the spec. > > > > Review of attachment 655401 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > Thanks for your update! I would give this r+ but I get a test failure: > > > > TEST-UNEXPECTED-FAIL | > > chrome://mochitests/content/browser/browser/devtools/styleeditor/test/ > > browser_styleeditor_init.js | stylesheet is unicode-aware. > > > > This always happens for me. > > I cannot reproduce this at all. Every time I run the mochitests, I get > complete success. I have started a try push to investigate this, at > https://tbpl.mozilla.org/?tree=Try&rev=430a0f7a10a2, but it will require > time, unless you are willing to investigate this issue yourself. The try push is orange on all systems... > > ::: browser/devtools/styleeditor/StyleEditor.jsm > > @@ +766,5 @@ > > > + * Source of a CSS stylesheet, loaded from file or cache. > > > + * @param string aChannelCharset > > > + * Charset of the source string if set by the HTTP channel. > > > + */ > > > + _decodeCSSCharset: function SE__decodeCSSCharset(aString, aChannelCharset) > > > > Does this method actually use aString? Would it be better to only return the > > charset name that should be used? > > > > Something like: > > charset = this._determineCSSCharset(theChannelCharsetIHave); > > string = this._convertToUnicode(string, charset); > > I feel against that, because this method doesn't have any use without the > call to _convertToUnicode(). Sure, I don't feel strong about this matter. Your choice. My reasoning: in your current approach one cannot check which character set was used for reading the source text. Later on, when you want to save the file, you will want to save in the same character set - not utf8 - and in those cases we will need to know the charset being used (if I am not mistaken). I do agree this is not something we need to bother right now. So, let it like it is now. > > @@ +804,5 @@ > > > + } > > > + > > > + if (sheet.ownerNode) { > > > + // step 4 (2 of 2): charset of referring document. > > > + if (sheet.ownerNode.ownerDocument.characterSet) { > > > > Why not roll these two if conditions into one? > > > > if (sheet.ownerNode && sheet.ownerNode.ownerDocument.characterset) { ... } > > You're right. It's going in my next patch. Thank you!

Assignee

Comment 18

•

12 years ago

(In reply to Mihai Sucan [:msucan] from comment #17) > (In reply to Thaddee Tyl [:espadrine] from comment #16) > > (In reply to Mihai Sucan [:msucan] from comment #15) > > > Thanks for your update! I would give this r+ but I get a test failure: > > > > > > TEST-UNEXPECTED-FAIL | > > > chrome://mochitests/content/browser/browser/devtools/styleeditor/test/ > > > browser_styleeditor_init.js | stylesheet is unicode-aware. > > > > > > This always happens for me. > > > > I cannot reproduce this at all. Every time I run the mochitests, I get > > complete success. I have started a try push to investigate this, at > > https://tbpl.mozilla.org/?tree=Try&rev=430a0f7a10a2, but it will require > > time, unless you are willing to investigate this issue yourself. > > The try push is orange on all systems... I wish I could tell you that it's obvious what mistake I made, and that it does fail on my machine, but running the tests on my machine systematically succeeds! If you have any idea why that may be, or how this test should be written, I'd love to hear it… ☹ In the meantime, I'll be debugging this issue with dump()s and try pushes.

Rob Campbell [:rc] (:robcee)

Assignee

Comment 19

•

12 years ago

Mihai: I start to understand why the test fails on your machine, and on try, but not on my machine. As you can see in the following trace available at that link: https://tbpl.mozilla.org/php/getParsedLog.php?id=14769729&tree=Try the text used as a test does contain a unicode smiley ☺. However, the testing framework, or the JS engine, isn't unicode-aware itself (except on my machine, I made special care of that a while ago). As a result, a simple dump of the smiley yields a unicode character interpreted as latin-1, decoded as unicode: âº. The testing system probably believes that the file is in latin-1, even though it was utf-8. I'm trying different approaches to circumvent this issue, otherwise I'll have to dig deep into the system, and I'll ask people from JS engine world.

Updated

•

12 years ago

Summary: Style editor brakes non-latin (national) encoded text → Style editor breaks non-latin (national) encoded text

Assignee

Comment 20

•

12 years ago

Attached patch Fix that implements all the steps of the spec. (obsolete) — Details — Splinter Review

I found a way to make the test work, so that's that. https://tbpl.mozilla.org/?tree=Try&rev=07fa912f50ff

Attachment #655401 - Attachment is obsolete: true

Attachment #656359 - Flags: review?(mihai.sucan)

Comment 21

•

12 years ago

Comment on attachment 656359 [details] [diff] [review] Fix that implements all the steps of the spec. Review of attachment 656359 [details] [diff] [review]: ----------------------------------------------------------------- Patch looks good. Thanks for your update! Giving it r+ with the comments below addressed. If this is green on the try servers, then please let me know so I can land the patch after you update it. Thanks! ::: browser/devtools/styleeditor/StyleEditor.jsm @@ +766,5 @@ > + * Source of a CSS stylesheet, loaded from file or cache. > + * @param string aChannelCharset > + * Charset of the source string if set by the HTTP channel. > + */ > + _decodeCSSCharset: function SE__decodeCSSCharset(aString, aChannelCharset) This is missing a @return description. @@ +829,5 @@ > + let converter = Cc["@mozilla.org/intl/scriptableunicodeconverter"] > + .createInstance(Ci.nsIScriptableUnicodeConverter); > + > + converter.charset = aCharset; > + return converter.ConvertToUnicode(aString); You had a try-catch wrapper for the entire _decodeCSSCharset() which is now gone. I'd like to know if ConvertToUnicode() can throw or if setting converter.charset to an unknown charset can throw? I looked at the wiki and it seems this could be the case. You might want to wrap these two lines in a try-catch and have the function return aString unmodified if an exception occurs. [1] https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIScriptableUnicodeConverter

Attachment #656359 - Flags: review?(mihai.sucan) → review+

Comment 22

•

12 years ago

(In reply to Thaddee Tyl [:espadrine] from comment #13) > (In reply to Mihai Sucan [:msucan] from comment #12) > > @@ +837,4 @@ > > > } > > > let source = NetUtil.readInputStreamToString(aStream, aStream.available()); > > > aStream.close(); > > > + this._onSourceLoad(source, ""); > > > > Do we always get an utf8 result from reading a file? Even if it's latin1 or > > some other encoding? > > From reading a file, we always get ISO-Latin-1. > That's why we need to convert it to Unicode. I've read the docs and they are, unfortunately, unclear (at least for me). I saw the latin1 mention, but that doesn't seem to explicitly say "you get latin1" or some other charset - it seemed to me the docs say that the file is read as if it's written using latin1 - but do you get latin1 back or utf8? We can have a follow up bug, if anyone discovers any problems with importing files from the disk - unless we spend more time here to analyze that case. I suggest we don't. Thanks for your patch!

Assignee

Comment 23

•

12 years ago

Attached patch [in-fx-team] Fix that implements all the steps of the spec. — Details — Splinter Review

I looked at the wiki, which at first glance didn't obviously have its methods throw. However it does seem like setting the charset can throw: https://mxr.mozilla.org/mozilla-central/source/intl/uconv/src/nsScriptableUConv.cpp#233 I guess the wiki can be unclear at times :) This patch addresses your review's comments.

Attachment #656359 - Attachment is obsolete: true

Loic

Comment 24

•

12 years ago

There is a similar issue with character encoding in Debugger, see bug 786169. Maybe you could backport this patch into Debugger too.

Comment 25

•

12 years ago

Comment on attachment 656495 [details] [diff] [review] [in-fx-team] Fix that implements all the steps of the spec. Thank you Thaddee! Landed: https://hg.mozilla.org/integration/fx-team/rev/a1adb4d46c3f

Attachment #656495 - Attachment description: Fix that implements all the steps of the spec. → [in-fx-team] Fix that implements all the steps of the spec.

Updated

•

12 years ago

Whiteboard: [fixed-in-fx-team]

https://hg.mozilla.org/mozilla-central/rev/a1adb4d46c3f

Comment 26

•

12 years ago

Status: ASSIGNED → RESOLVED

Closed: 12 years ago

Resolution: --- → FIXED

Ryan VanderMeulen [:RyanVM]

Comment 27

•

12 years ago

This was causing Windows debug mochitest-other crashes on fx-team, and (surprise!) it caused crashes on mozilla-central as well after being merged there. Backed out. https://hg.mozilla.org/mozilla-central/rev/f972f1a71e7e https://tbpl.mozilla.org/php/getParsedLog.php?id=14850626&tree=Firefox TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/styleeditor/test/browser_styleeditor_reopen.js | Exited with code -1073741819 during test run INFO | automation.py | Application ran for: 0:30:06.094000 INFO | automation.py | Reading PID log: c:\docume~1\cltbld\locals~1\temp\tmp2ssoxlpidlog ==> process 3820 launched child process 3040 ==> process 3820 launched child process 4032 INFO | automation.py | Checking for orphan process with PID: 3040 INFO | automation.py | Checking for orphan process with PID: 4032 Downloading symbols from: http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-win32-debug/1346352224/firefox-18.0a1.en-US.win32.crashreporter-symbols.zip PROCESS-CRASH | chrome://mochitests/content/browser/browser/devtools/styleeditor/test/browser_styleeditor_reopen.js | application crashed (minidump found) Crash dump filename: c:\docume~1\cltbld\locals~1\temp\tmptuqrgy\minidumps\daafc9c5-534d-40c4-95f0-d42841645394.dmp Operating system: Windows NT 5.1.2600 Service Pack 2 CPU: x86 GenuineIntel family 6 model 23 stepping 10 2 CPUs Crash reason: EXCEPTION_ACCESS_VIOLATION_READ Crash address: 0xffffffffdddddddd Thread 0 (crashed) 0 xul.dll!nsXMLNameSpaceMap::FindNameSpaceID(nsIAtom *) [nsXMLNameSpaceMap.cpp:ee7a3bddfe5f : 82 + 0x2] eip = 0x02086550 esp = 0x0012caa0 ebp = 0x0012caa4 ebx = 0x1364ae88 esi = 0xdddddddd edi = 0x08c77cd0 eax = 0x00000000 ecx = 0x094f0070 edx = 0x0000002f efl = 0x00010202 Found by: given as instruction pointer in context 1 xul.dll!`anonymous namespace'::CSSParserImpl::SetDefaultNamespaceOnSelector(nsCSSSelector &) [nsCSSParser.cpp:ee7a3bddfe5f : 9812 + 0x6] eip = 0x01eb3a5c esp = 0x0012caac ebp = 0x0012cab0 Found by: call frame info 2 xul.dll!`anonymous namespace'::CSSParserImpl::ParseTypeOrUniversalSelector(int &,nsCSSSelector &,bool) [nsCSSParser.cpp:ee7a3bddfe5f : 3009 + 0x5] eip = 0x01eb7cfd esp = 0x0012cab8 ebp = 0x0012cb60 Found by: call frame info 3 xul.dll!`anonymous namespace'::CSSParserImpl::ParseSelector(nsCSSSelectorList *,wchar_t) [nsCSSParser.cpp:ee7a3bddfe5f : 3796 + 0x1c] eip = 0x01ebac73 esp = 0x0012cb68 ebp = 0x0012cb94 Found by: call frame info 4 xul.dll!`anonymous namespace'::CSSParserImpl::ParseSelectorGroup(nsCSSSelectorList * &) [nsCSSParser.cpp:ee7a3bddfe5f : 2850 + 0xa] eip = 0x01ebb9fc esp = 0x0012cb9c ebp = 0x0012cbb8 Found by: call frame info 5 xul.dll!`anonymous namespace'::CSSParserImpl::ParseSelectorList(nsCSSSelectorList * &,wchar_t) [nsCSSParser.cpp:ee7a3bddfe5f : 2785 + 0xa] eip = 0x01ebbd09 esp = 0x0012cbc0 ebp = 0x0012cbcc Found by: call frame info 6 xul.dll!`anonymous namespace'::CSSParserImpl::ParseRuleSet(void (*)(mozilla::css::Rule *,void *),void *,bool) [nsCSSParser.cpp:ee7a3bddfe5f : 2748 + 0x12] eip = 0x01ec5425 esp = 0x0012cbd4 ebp = 0x0012cbec Found by: call frame info 7 xul.dll!`anonymous namespace'::CSSParserImpl::ParseSheet(nsAString_internal const &,nsIURI *,nsIURI *,nsIPrincipal *,unsigned int,bool) [nsCSSParser.cpp:ee7a3bddfe5f : 934 + 0x9] eip = 0x01ec6331 esp = 0x0012cbf4 ebp = 0x0012cc14 Found by: call frame info 8 xul.dll!nsCSSStyleSheet::ParseSheet(nsAString_internal const &) [nsCSSStyleSheet.cpp:ee7a3bddfe5f : 2167 + 0x1e] eip = 0x01edd788 esp = 0x0012cc1c ebp = 0x0012cc54

Status: RESOLVED → REOPENED

Resolution: FIXED → ---

Whiteboard: [fixed-in-fx-team]

Ryan VanderMeulen [:RyanVM]

Comment 28

•

12 years ago

And for the record, failing in the same test as another known intermittent failure bug does NOT make it the same failure.

Ryan VanderMeulen [:RyanVM]

Assignee

Comment 29

•

12 years ago

Ryan: I really thought this was the intermittent failure. FWIW, this crash is completely unrelated to the patch discussed here. A bug for it has already been reported long before I started working on this patch: https://bugzilla.mozilla.org/show_bug.cgi?id=781032

Comment 30

•

12 years ago

Sounds like this patch is tickling that crash. Should make it easier to debug! :) Regardless, crashes != timeouts. Furthermore, it should make you suspicious when you look at the history of that intermittent failure and see how little it happens and suddenly Windows debug m-oth goes perma-orange after landing. IMO, it was a bit reckless to merge this over to m-c without any green m-oth runs on Windows debug.

Comment 31

•

12 years ago

Backed out on fx-team as well.

Assignee

Updated

•

12 years ago

Depends on: 781032

Panos Astithas (he/him) [:past] (please ni?)

Assignee

Comment 32

•

12 years ago

Try push with the patch I provided in bug 781032: https://tbpl.mozilla.org/?tree=Try&rev=d0fc5ca59325

Updated

•

12 years ago

Target Milestone: --- → Firefox 18

Panos Astithas (he/him) [:past] (please ni?)

Updated

•

12 years ago

Target Milestone: Firefox 18 → ---

Comment 33

•

12 years ago

Comment on attachment 656495 [details] [diff] [review] [in-fx-team] Fix that implements all the steps of the spec. Re-landed: https://hg.mozilla.org/integration/fx-team/rev/c2e8166ec5d7 Thank you Thaddee for the crash fix and for the updated try push.

Updated

•

12 years ago

Whiteboard: [fixed-in-fx-team]

Comment 34

•

12 years ago

(In reply to Mihai Sucan [:msucan] from comment #33) > Comment on attachment 656495 [details] [diff] [review] > [in-fx-team] Fix that implements all the steps of the spec. > > Re-landed: > https://hg.mozilla.org/integration/fx-team/rev/c2e8166ec5d7 > > Thank you Thaddee for the crash fix and for the updated try push. Same orange on fx-team. It doesn't seem that bug 781032 got into fx-team yet. A straight pull from m-c should suffice, probably.