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

Status: RESOLVED FIXED

(Core :: XSLT, defect)

Description

[Carsten Book [:Tomcat]]

Attachment: 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

Comment 1

[Carsten Book [:Tomcat]]

[Tracking Requested - why for this release]

crashes opt and debug builds trunk -> beta

bill, markus: can you take a look ?

Comment 2

[Bob Clary [:bc] (inactive)]

Attachment: asan segv nsDocument::SetScopeObject(nsIGlobalObject*)

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Ehsan, Michael, do you guys have any advice? I've never even heard of these functions.

Comment 4

[(no longer active)]

I'll debug.

Comment 5

[(no longer active)]

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

Comment 6

[(no longer active)]

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.

Comment 7

[(no longer active)]

(Thankfully release is unaffected.)

Comment 8

[(no longer active)]

I just confirmed that fixing bug 1334281 will make our existing crashtests catch this.

Comment 9

[Julien Cristau [:jcristau]]

Tracking for 52 and up, per comment 6.

Comment 10

[(no longer active)]

Because of bug 1334281 comment 5, I think a good branch fix for this would be removing the assertion.

Comment 11

[(no longer active)]

Attachment: Only set the script handling object for XSLT result documents after setting up the principal

Comment 12

[(no longer active)]

Attachment: Remove the release assertion for docgroup keys matching until we fix the code to adhere to this (branch fix)

Comment 13

[(no longer active)]

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.

Comment 14

[Julien Cristau [:jcristau]]

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+

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0411363a0dc7d350ac731edc047e3c6f3ef51c1d

Comment 16

[Carsten Book [:Tomcat]]

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

Comment 17

[Carsten Book [:Tomcat]]

backed out also from aurora for causing bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=72985697&repo=mozilla-aurora

Comment 18

[(no longer active)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e72aaa3b621e51a2563cf72335813b1a05bd7c23

Comment 19

[(no longer active)]

https://hg.mozilla.org/releases/mozilla-beta/rev/0f339c1e154f75c484fe2fac507a9a225990d212

Comment 20

[Carsten Book [:Tomcat]]

backed out from aurora for bustage still https://treeherder.mozilla.org/logviewer.html#?job_id=72993930&repo=mozilla-aurora

Comment 21

[(no longer active)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/40425a7cb3569597c1277203c8c2d0109bb78468

Comment 22

[(no longer active)]

Peter, do you think you'll get a chance to review this?  If not can you please suggest someone else?

Comment 23

[(no longer active)]

Thanks for the review!

Comment 24

[Pulsebot]

Pushed by [e-mail]:
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

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/28974f2c413c