Crash when loading any XSLT file from most websites with Assertion failure: mDocGroup->MatchesKey(docGroupKey), at nsDocument.cpp:2985

RESOLVED FIXED in Firefox 52

Status

()

Core
XSLT
--
critical
RESOLVED FIXED
7 months ago
7 months ago

People

(Reporter: Tomcat, Assigned: Ehsan)

Tracking

(Blocks: 1 bug, {assertion, crash, regression})

unspecified
mozilla54
assertion, crash, regression
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox52+ fixed, firefox53+ fixed, firefox54+ fixed)

Details

(crash signature, URL)

Attachments

(4 attachments)

(Reporter)

Description

7 months ago
Created attachment 8830636 [details]
bughunter stack

Assertion failure: mDocGroup->MatchesKey(docGroupKey), at c:/builds/moz2_slave/m-cen-w32-d-000000000000000000/build/src/dom/base/nsDocument.cpp:2985

found by bughunter reproduced with latest mozilla debug trunk tinderbox windows build on win7

Bughunter data show that Linux and Windows are affected as well as all branches like aurora,beta and trunk

Steps to reproduce:
-> Load https://www.checkcolorado.com/caseDisplay.php?case_id=2005T104174%28County%29&county_code=LAR
--> Crash on load

also crashes opt builds like https://crash-stats.mozilla.com/report/index/d8d90bf7-7b92-4712-ab44-6ea162170126
(Reporter)

Comment 1

7 months ago
[Tracking Requested - why for this release]

crashes opt and debug builds trunk -> beta

bill, markus: can you take a look ?
tracking-firefox52: --- → ?
tracking-firefox53: --- → ?
tracking-firefox54: --- → ?
Flags: needinfo?(wmccloskey)
Flags: needinfo?(mstange)

Comment 2

7 months ago
Created attachment 8830825 [details]
asan segv nsDocument::SetScopeObject(nsIGlobalObject*)
Flags: needinfo?(wmccloskey)
Ehsan, Michael, do you guys have any advice? I've never even heard of these functions.
Flags: needinfo?(mstange)
Flags: needinfo?(michael)
Flags: needinfo?(ehsan)
I'll debug.
Flags: needinfo?(ehsan)
The case at hand here is an XSLT document which uses the text output method.  We set up the DocGroup here <http://searchfox.org/mozilla-central/rev/7da3c9dcf467964f2fb82f3a4c63972ee79bf696/dom/xslt/xslt/txMozillaTextOutput.cpp#152> but at that point we have the null principal set on mDocument so we get a DocGroup with an empty key, and then we get to here <http://searchfox.org/mozilla-central/rev/7da3c9dcf467964f2fb82f3a4c63972ee79bf696/dom/xslt/xslt/txMozillaTextOutput.cpp#157> where we call ResetToURI which causes us to get the correct principal under this call stack:

    frame #1: 0x000000011182783f XUL`nsDocument::SetPrincipal(this=0x000000010bbe3000, aNewPrincipal=0x000000010becfce0) + 335 at nsDocument.cpp:2858
    frame #2: 0x0000000111822012 XUL`nsDocument::ResetToURI(this=0x000000010bbe3000, aURI=0x000000010bab3020, aLoadGroup=0x000000010771f020, aPrincipal=0x000000010becfce0) + 1442 at nsDocument.cpp:2092
    frame #3: 0x0000000113bb34da XUL`mozilla::dom::XMLDocument::ResetToURI(this=0x000000010bbe3000, aURI=0x000000010bab3020, aLoadGroup=0x000000010771f020, aPrincipal=0x000000010becfce0) + 138 at XMLDocument.cpp:274
    frame #4: 0x000000011182188e XUL`nsDocument::Reset(this=0x000000010bbe3000, aChannel=0x000000010bbb5800, aLoadGroup=0x000000010771f020) + 334 at nsDocument.cpp:1976
    frame #5: 0x0000000113bb3440 XUL`mozilla::dom::XMLDocument::Reset(this=0x000000010bbe3000, aChannel=0x000000010bbb5800, aLoadGroup=0x000000010771f020) + 48 at XMLDocument.cpp:261
    frame #6: 0x0000000113bc9195 XUL`URIUtils::ResetWithSource(aNewDoc=0x000000010bbe3000, aSourceNode=0x000000010f347000) + 565 at txURIUtils.cpp:77
    frame #7: 0x0000000113c0fd17 XUL`txMozillaTextOutput::createResultDocument(this=0x000000010a70f100, aSourceDocument=0x000000010f347000, aLoadedAsData=false) + 967 at txMozillaTextOutput.cpp:157
    frame #8: 0x0000000113c1756c XUL`txToDocHandlerFactory::createHandlerWith(this=0x00007fff58c778f0, aFormat=0x0000000128257ee0, aHandler=0x00007fff58c77828) + 588 at txMozillaXSLTProcessor.cpp:118
    frame #9: 0x0000000113bfd2b6 XUL`txExecutionState::init(this=0x00007fff58c77968, aNode=0x0000000128194da0, aGlobalParams=0x000000010be60c50) + 198 at txExecutionState.cpp:116

At this point we get the correct principal and compute a non-empty key, and the assertion fails because we have already set up DocGroup.

What's even more interesting is that we have managed ~15 years since this code landed in bug 155578 without a single test case for this.  :(
Assignee: nobody → ehsan
Component: DOM: Content Processes → XSLT
Flags: needinfo?(michael)
See Also: → bug 1334281
Depends on: 1334320
No longer depends on: 1334320
See Also: → bug 1334320
See Also: → bug 1334322
Note that due to bug 1334322, this bug currently won't get triggered when running reftests/crashtest, and due to bug 1334320 it also won't get triggered from reftests/crashtests that have the HTTP annotation in the manifest (since they're served from http://localhost).  Otherwise this would crash us for any XSLT document being loaded.  :(  See for example bp-08817b04-8f18-426f-9e2f-82e2d2170126.

I'll fix those bugs first to see if my hypothesis above is correct.  But this fix should be backported to 52.
Blocks: 1303196
Crash Signature: [@ nsDocument::SetScopeObject]
status-firefox52: --- → affected
status-firefox53: --- → affected
status-firefox54: --- → affected
Keywords: regression
Summary: Assertion failure: mDocGroup->MatchesKey(docGroupKey), at nsDocument.cpp:2985 → Crash when loading any XSLT file from most websites with Assertion failure: mDocGroup->MatchesKey(docGroupKey), at nsDocument.cpp:2985
(Thankfully release is unaffected.)
I just confirmed that fixing bug 1334281 will make our existing crashtests catch this.
See Also: bug 1334320, bug 1334322
Tracking for 52 and up, per comment 6.
tracking-firefox52: ? → +
tracking-firefox53: ? → +
tracking-firefox54: ? → +
Blocks: 1334281
See Also: bug 1334281
Because of bug 1334281 comment 5, I think a good branch fix for this would be removing the assertion.
Created attachment 8831156 [details] [diff] [review]
Only set the script handling object for XSLT result documents after setting up the principal
Attachment #8831156 - Flags: review?(peterv)
Created attachment 8831158 [details] [diff] [review]
Remove the release assertion for docgroup keys matching until we fix the code to adhere to this (branch fix)
Attachment #8831158 - Flags: review?(wmccloskey)

Updated

7 months ago
Attachment #8831158 - Flags: review?(wmccloskey) → review+
Comment on attachment 8831158 [details] [diff] [review]
Remove the release assertion for docgroup keys matching until we fix the code to adhere to this (branch fix)

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1303196.
[User impact if declined]: Crash when visiting most web-hosted XSLT pages.
[Is this code covered by automated tests?]: This patch doesn't need tests.
[Has the fix been verified in Nightly?]: No, this is a branch specific patch disabling an assertion.  We'll land a real fix on trunk.
[Needs manual test from QE? If yes, steps to reproduce]: Np.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: See above.
[String changes made/needed]: None.
Attachment #8831158 - Flags: approval-mozilla-beta?
Attachment #8831158 - Flags: approval-mozilla-aurora?
Comment on attachment 8831158 [details] [diff] [review]
Remove the release assertion for docgroup keys matching until we fix the code to adhere to this (branch fix)

remove an assertion to fix crash, aurora53+, beta52+
Attachment #8831158 - Flags: approval-mozilla-beta?
Attachment #8831158 - Flags: approval-mozilla-beta+
Attachment #8831158 - Flags: approval-mozilla-aurora?
Attachment #8831158 - Flags: approval-mozilla-aurora+
(Reporter)

Comment 15

7 months ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/0411363a0dc7d350ac731edc047e3c6f3ef51c1d
status-firefox53: affected → fixed
(Reporter)

Comment 16

7 months ago
has problems to apply to beta:

grafting 386896:0411363a0dc7 "Bug 1334047 - Remove the release assertion for docgroup keys matching until we fix the code to adhere to this (branch fix); r=billm a=jcristau"
merging dom/base/nsDocument.cpp
warning: conflicts while merging dom/base/nsDocument.cpp! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')
Tomcats-MacBook-Pro-300:mozilla-central Tomcat
Flags: needinfo?(ehsan)
(Reporter)

Comment 17

7 months ago
backed out also from aurora for causing bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=72985697&repo=mozilla-aurora
status-firefox53: fixed → affected
https://hg.mozilla.org/releases/mozilla-aurora/rev/e72aaa3b621e51a2563cf72335813b1a05bd7c23
status-firefox53: affected → fixed
Flags: needinfo?(ehsan)
https://hg.mozilla.org/releases/mozilla-beta/rev/0f339c1e154f75c484fe2fac507a9a225990d212
status-firefox52: affected → fixed
(Reporter)

Comment 20

7 months ago
backed out from aurora for bustage still https://treeherder.mozilla.org/logviewer.html#?job_id=72993930&repo=mozilla-aurora
status-firefox53: fixed → affected
Flags: needinfo?(ehsan)
https://hg.mozilla.org/releases/mozilla-aurora/rev/40425a7cb3569597c1277203c8c2d0109bb78468
status-firefox53: affected → fixed
Flags: needinfo?(ehsan)
Flags: qe-verify-
Peter, do you think you'll get a chance to review this?  If not can you please suggest someone else?
Flags: needinfo?(peterv)
Attachment #8831156 - Flags: review?(peterv) → review+
Thanks for the review!
Flags: needinfo?(peterv)

Comment 24

7 months ago
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/28974f2c413c
Only set the script handling object for XSLT result documents after setting up the principal; r=peterv

Comment 25

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/28974f2c413c
Status: NEW → RESOLVED
Last Resolved: 7 months ago
status-firefox54: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.