781032 - Crash in nsXMLNameSpaceMap::FindNameSpaceID using DevTools' Style editor

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Thaddee Tyl [:espadrine]]

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

That's pretty odd.  :(

I assume this isn't reproducible?  Do you still have this in a debugger?

Comment 2

[Boris Zbarsky [:bzbarsky]]

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.

Comment 3

[Thaddee Tyl [:espadrine]]

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

Comment 4

[Thaddee Tyl [:espadrine]]

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.

Comment 5

[Thaddee Tyl [:espadrine]]

Attachment: Backtrace

Comment 6

[Thaddee Tyl [:espadrine]]

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.

Comment 7

[Boris Zbarsky [:bzbarsky]]

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

Comment 8

[Thaddee Tyl [:espadrine]]

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?

Comment 9

[Boris Zbarsky [:bzbarsky]]

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

Comment 10

[Thaddee Tyl [:espadrine]]

Attachment: Crash fix in CSSParserImpl::SetDefaultNamespaceOnSelector

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 11

[Boris Zbarsky [:bzbarsky]]

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?

Comment 12

[Thaddee Tyl [:espadrine]]

(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?

Comment 13

[Thaddee Tyl [:espadrine]]

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.

Comment 14

[Thaddee Tyl [:espadrine]]

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

Comment 15

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 16

[Boris Zbarsky [:bzbarsky]]

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

Comment 17

[Thaddee Tyl [:espadrine]]

Attachment: Fix to create the nsCSSParser at a valid spot.

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 18

[Boris Zbarsky [:bzbarsky]]

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...

Comment 19

[Thaddee Tyl [:espadrine]]

Attachment: Fix to create the nsCSSParser at a valid spot.

Fair enough!

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

Comment 20

[Boris Zbarsky [:bzbarsky]]

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?

Comment 21

[Thaddee Tyl [:espadrine]]

Attachment: Fix to create the nsCSSParser at a valid spot.

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 22

[Boris Zbarsky [:bzbarsky]]

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.

Comment 23

[Thaddee Tyl [:espadrine]]

Attachment: Fix to create the nsCSSParser at a valid spot.

True.

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

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Green on Try.
https://tbpl.mozilla.org/?tree=Try&rev=5bc744d42d24

https://hg.mozilla.org/integration/mozilla-inbound/rev/c6768c151b64

Comment 25

[Ryan VanderMeulen [:RyanVM]]

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