Closed Bug 781032 Opened 9 years ago Closed 9 years ago

Crash in nsXMLNameSpaceMap::FindNameSpaceID using DevTools' Style editor

Categories

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

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: espadrine, Assigned: espadrine)

References

Details

(Keywords: crash)

Attachments

(2 files, 4 obsolete files)

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…
Severity: normal → critical
Keywords: crash
Summary: Crash in nsXMLNameSpaceMap::FindNameSpaceID → Crash in nsXMLNameSpaceMap::FindNameSpaceID using DevTools' Style editor
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.
OS: Linux → All
Hardware: x86_64 → All
Attached file Backtrace
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.
Assignee: nobody → thaddee.tyl
Attachment #658160 - Flags: review?(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?
(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?
Blocks: 747820
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.
> 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.
Attachment #658160 - Attachment is obsolete: true
Attachment #658160 - Flags: review?(bzbarsky)
Attachment #658238 - Flags: review?(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...
Attachment #658238 - Flags: review?(bzbarsky) → review-
Fair enough!

Try at https://tbpl.mozilla.org/?tree=Try&rev=6310c88c5124.
Attachment #658238 - Attachment is obsolete: true
Attachment #658293 - Flags: review?(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?
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?
Attachment #658293 - Attachment is obsolete: true
Attachment #658293 - Flags: review?(bzbarsky)
Attachment #658306 - Flags: review?(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.
Attachment #658306 - Flags: review?(bzbarsky) → review+
Attachment #658315 - Flags: checkin?
Keywords: checkin-needed
Attachment #658315 - Flags: checkin?
https://hg.mozilla.org/mozilla-central/rev/c6768c151b64
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Duplicate of this bug: 789843
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.