Crash in nsXMLNameSpaceMap::FindNameSpaceID using DevTools' Style editor

RESOLVED FIXED in mozilla18

Status

()

defect
--
critical
RESOLVED FIXED
7 years ago
3 months ago

People

(Reporter: espadrine, Assigned: espadrine)

Tracking

({crash})

unspecified
mozilla18
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

Assignee

Description

7 years ago
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.
Assignee

Comment 3

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

Updated

7 years ago
Severity: normal → critical
Keywords: crash
Summary: Crash in nsXMLNameSpaceMap::FindNameSpaceID → Crash in nsXMLNameSpaceMap::FindNameSpaceID using DevTools' Style editor
Assignee

Comment 4

7 years ago
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
Assignee

Comment 5

7 years ago
Posted file Backtrace
Assignee

Comment 6

7 years ago
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!
Assignee

Comment 8

7 years ago
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).
Assignee

Comment 10

7 years ago
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?
Assignee

Comment 12

7 years ago
(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?
Assignee

Updated

7 years ago
Blocks: 747820
Assignee

Comment 13

7 years ago
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.
Assignee

Comment 17

7 years ago
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-
Assignee

Comment 19

7 years ago
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?
Assignee

Comment 21

7 years ago
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+
Assignee

Updated

7 years ago
Attachment #658315 - Flags: checkin?
Assignee

Updated

7 years ago
Keywords: checkin-needed
Attachment #658315 - Flags: checkin?
https://hg.mozilla.org/mozilla-central/rev/c6768c151b64
Status: NEW → RESOLVED
Closed: 7 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.