This happened while fiddling with the DevTools' Style editor. The backtrace shows that it went through parsing a selector to getting an ID. Backtrace: http://pastebin.mozilla.org/1743448.

That's pretty odd. :( I assume this isn't reproducible? Do you still have this in a debugger?

In particular, it looks like something corrupted your memory; that's the only way I can see an IndexOf on nsTArray with that comparator crashing.

Unfortunately, I don't have it in the debugger anymore…

It is (unfortunately) reproducible on windows debug on the try server, with a recent patch that subsequently got backed out, as discussed here: https://bugzilla.mozilla.org/show_bug.cgi?id=747820#c27 Notably, the ESI register is oddly similar to the access violation address.

I believe I can trigger this crash with inIDOMUtils.parseStyleSheet(), which systematically crashes when dealing with a sheet that contains utf-8 (non-ascii) characters.

If you can give me a way to actually reproduce, that would be awesome!

Either that or a patch. I have never submitted a patch for this corner of the tree. Can you review it, or suggest me a reviewer?

I can review, sure. Alternately, depending on the exact place being patched either dbaron (for a style system change) or one of the DOM peers (for the nsXMLNameSpaceMap).

The issue was caused by the use of `*mNameSpaceMap` in nsCSSParser.cpp, which is a weak link, while `mSheet.get().mInner.mNameSpaceMap.get()` (or `mSheet.GetNameSpaceMap()` for short) was the strong link. As a result, in the edge case I triggered, `mNameSpaceMap->mNameSpaces.mHdr` contained 0x5a5a5a5a5a5a5a5a, while the strong link was a null pointer.

Comment on attachment 658160 [details] [diff] [review] Crash fix in CSSParserImpl::SetDefaultNamespaceOnSelector Ah, because parseSheet nukes the mNameSpaceMap altogether. I guess this could also happen with RebuildNameSpaces. But the real problem, I think, is that CSSParserImpl::SetStyleSheet doesn't reset mNameSpaceMap if aSheet == mSheet. It probably should. Does doing that, but leaving SetDefaultNamespaceOnSelector as-is fix the bug too?

(In reply to Boris Zbarsky (:bz) from comment #11) > But the real problem, I think, is that CSSParserImpl::SetStyleSheet doesn't > reset mNameSpaceMap if aSheet == mSheet. It probably should. Does doing > that, but leaving SetDefaultNamespaceOnSelector as-is fix the bug too? It would make sense, but it doesn't seem to work. Is going through CSSParserImpl::SetStyleSheet the required way to change the stylesheet?

To be clear: we nuke mNameSpaceMap at layout/style/nsCSSStyleSheet.cpp:2162, but CSSParserImpl::SetStyleSheet never gets called by CSSParser::ParseSheet. I don't think it should. The style sheet itself doesn't change, it parses a different input and internally replaces everything. I believe the patch I have submitted is the best way to fix this bug.

Corresponding try push: https://tbpl.mozilla.org/?tree=Try&rev=c7b1cdf39b48

> To be clear: we nuke mNameSpaceMap at layout/style/nsCSSStyleSheet.cpp:2162, but > CSSParserImpl::SetStyleSheet never gets called by CSSParser::ParseSheet. Ah. So. That looks like a bug in nsCSSStyleSheet::ParseSheet. It should create the on-stack nsCSSParser after it's finished gutting is insides, so right before it does the ParseSheet() call.

And I meant "its own insides", of course.

Create the nsCSSParser on-stack just before parsing (and after the style sheet guts its own insides :) does work. Try run at https://tbpl.mozilla.org/?tree=Try&rev=a4f82ba9a584.

Comment on attachment 658238 [details] [diff] [review] Fix to create the nsCSSParser at a valid spot. I think we still need to reget mNameSpaceMap in SetStyleSheet, in case RebuildNameSpaces happened on the stylesheet...

Fair enough! Try at https://tbpl.mozilla.org/?tree=Try&rev=6310c88c5124.

Comment on attachment 658293 [details] [diff] [review] Fix to create the nsCSSParser at a valid spot. Don't you need to reget the mNameSpaceMap from the new sheet?

If my understanding of the situation is ok, then occasionally mNameSpaceMap breaks, and RebuildNameSpaces is one of these occasions. Will this change ensure that we re-wire it correctly in all cases?

Comment on attachment 658306 [details] [diff] [review] Fix to create the nsCSSParser at a valid spot. You probably need to do |else if (mSheet)| to be safe, but r=me with that.

True. Try push: https://tbpl.mozilla.org/?tree=Try&rev=eba5cce4eabd

Green on Try. https://tbpl.mozilla.org/?tree=Try&rev=5bc744d42d24 https://hg.mozilla.org/integration/mozilla-inbound/rev/c6768c151b64