DOMParser doesn't set the principal of the documents that it creates early enough

RESOLVED FIXED in Firefox 54

Status

()

Core
DOM
RESOLVED FIXED
10 months ago
10 months ago

People

(Reporter: Away for a while, Assigned: Away for a while)

Tracking

unspecified
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

10 months ago
Example:

[task 2017-02-01T06:21:52.659643Z] 06:21:52     INFO - TEST-START | dom/base/test/chrome/test_bug429785.xul
[task 2017-02-01T06:21:52.676822Z] 06:21:52     INFO - ++DOMWINDOW == 54 (0xce2c9800) [pid = 3570] [serial = 54] [outer = 0xce234c00]
[task 2017-02-01T06:21:52.693417Z] 06:21:52     INFO - Assertion failure: mDocGroup->MatchesKey(docGroupKey), at /home/worker/workspace/build/src/dom/base/nsDocument.cpp:2985
[task 2017-02-01T06:22:20.018812Z] 06:22:20     INFO - #01: nsIDocument::GetDocGroup [dom/base/nsDocument.cpp:2985]
[task 2017-02-01T06:22:20.020004Z] 06:22:20     INFO - 
[task 2017-02-01T06:22:20.020699Z] 06:22:20     INFO - #02: nsDocument::SetPrincipal [dom/base/nsDocument.cpp:2969]
[task 2017-02-01T06:22:20.020762Z] 06:22:20     INFO - 
[task 2017-02-01T06:22:20.021601Z] 06:22:20     INFO - #03: mozilla::dom::DOMParser::ParseFromStream [dom/base/DOMParser.cpp:286]
[task 2017-02-01T06:22:20.022247Z] 06:22:20     INFO - 
[task 2017-02-01T06:22:20.022431Z] 06:22:20     INFO - #04: mozilla::dom::DOMParser::ParseFromString [dom/base/DOMParser.cpp:128]
[task 2017-02-01T06:22:20.023136Z] 06:22:20     INFO - 
[task 2017-02-01T06:22:20.023327Z] 06:22:20     INFO - #05: mozilla::dom::DOMParser::ParseFromString [dom/bindings/ErrorResult.h:319]
[task 2017-02-01T06:22:20.024482Z] 06:22:20     INFO - 
[task 2017-02-01T06:22:20.025197Z] 06:22:20     INFO - #06: mozilla::dom::DOMParserBinding::parseFromString [obj-firefox/dom/bindings/DOMParserBinding.cpp:75]
[task 2017-02-01T06:22:20.025356Z] 06:22:20     INFO - 
[task 2017-02-01T06:22:20.025976Z] 06:22:20     INFO - #07: mozilla::dom::GenericBindingMethod [dom/bindings/BindingUtils.cpp:2961]
[task 2017-02-01T06:22:20.026634Z] 06:22:20     INFO - 
[task 2017-02-01T06:22:20.027328Z] 06:22:20     INFO - #08: js::CallJSNative [js/src/jscntxtinlines.h:263]
[task 2017-02-01T06:22:20.027968Z] 06:22:20     INFO - 
[task 2017-02-01T06:22:20.028965Z] 06:22:20     INFO - #09: js::InternalCallOrConstruct [js/src/vm/Interpreter.cpp:460]
[task 2017-02-01T06:22:20.030454Z] 06:22:20     INFO - 
[task 2017-02-01T06:22:20.032086Z] 06:22:20     INFO - #10: InternalCall [js/src/vm/Interpreter.cpp:506]
[task 2017-02-01T06:22:20.033524Z] 06:22:20     INFO - 
[task 2017-02-01T06:22:20.035164Z] 06:22:20     INFO - #11: Interpret [js/src/vm/Interpreter.cpp:2957]
[task 2017-02-01T06:22:20.036606Z] 06:22:20     INFO - 
[task 2017-02-01T06:22:20.038187Z] 06:22:20     INFO - #12: js::RunScript [js/src/vm/Interpreter.cpp:406]
[task 2017-02-01T06:22:20.039122Z] 06:22:20     INFO - 
[task 2017-02-01T06:22:20.040759Z] 06:22:20     INFO - #13: js::ExecuteKernel [js/src/vm/Interpreter.h:248]
[task 2017-02-01T06:22:20.042371Z] 06:22:20     INFO - 
[task 2017-02-01T06:22:20.044189Z] 06:22:20     INFO - #14: js::Execute [js/src/vm/Interpreter.cpp:720]
[task 2017-02-01T06:22:20.045795Z] 06:22:20     INFO - 
[task 2017-02-01T06:22:20.047091Z] 06:22:20     INFO - #15: ExecuteScript [js/src/jsapi.cpp:4429]
[task 2017-02-01T06:22:20.048182Z] 06:22:20     INFO - 
[task 2017-02-01T06:22:20.049571Z] 06:22:20     INFO - #16: JS::CloneAndExecuteScript [js/src/jsapi.cpp:4492]
[task 2017-02-01T06:22:20.050674Z] 06:22:20     INFO - 
[task 2017-02-01T06:22:20.051969Z] 06:22:20     INFO - #17: mozilla::dom::XULDocument::ExecuteScript [js/public/RootingAPI.h:795]
[task 2017-02-01T06:22:20.053036Z] 06:22:20     INFO - 
[task 2017-02-01T06:22:20.054191Z] 06:22:20     INFO - #18: mozilla::dom::XULDocument::ResumeWalk [dom/xul/XULDocument.cpp:2863]
[task 2017-02-01T06:22:20.055689Z] 06:22:20     INFO - 
[task 2017-02-01T06:22:20.056728Z] 06:22:20     INFO - #19: mozilla::dom::XULDocument::EndLoad [dom/xul/XULDocument.cpp:561]
[task 2017-02-01T06:22:20.058163Z] 06:22:20     INFO - 
[task 2017-02-01T06:22:20.059750Z] 06:22:20     INFO - #20: XULContentSinkImpl::DidBuildModel [xpcom/base/nsCOMPtr.h:600]
[task 2017-02-01T06:22:20.061148Z] 06:22:20     INFO - 
[task 2017-02-01T06:22:20.062724Z] 06:22:20     INFO - #21: nsParser::DidBuildModel [parser/htmlparser/nsParser.cpp:908]
[task 2017-02-01T06:22:20.064171Z] 06:22:20     INFO - 
[task 2017-02-01T06:22:20.065998Z] 06:22:20     INFO - #22: nsParser::ResumeParse [parser/htmlparser/nsParser.cpp:1508]
[task 2017-02-01T06:22:20.067514Z] 06:22:20     INFO - 
[task 2017-02-01T06:22:20.068564Z] 06:22:20     INFO - #23: nsParser::OnStopRequest [parser/htmlparser/nsParser.cpp:1880]
[task 2017-02-01T06:22:20.069940Z] 06:22:20     INFO - 
[task 2017-02-01T06:22:20.071513Z] 06:22:20     INFO - #24: nsDocumentOpenInfo::OnStopRequest [uriloader/base/nsURILoader.cpp:338]
[task 2017-02-01T06:22:20.072925Z] 06:22:20     INFO - 
[task 2017-02-01T06:22:20.074474Z] 06:22:20     INFO - #25: nsBaseChannel::OnStopRequest [netwerk/base/nsBaseChannel.cpp:827]
[task 2017-02-01T06:22:20.075918Z] 06:22:20     INFO - 
[task 2017-02-01T06:22:20.076953Z] 06:22:20     INFO - #26: nsInputStreamPump::OnStateStop [netwerk/base/nsInputStreamPump.cpp:715]
[task 2017-02-01T06:22:20.078375Z] 06:22:20     INFO - 
[task 2017-02-01T06:22:20.079836Z] 06:22:20     INFO - #27: nsInputStreamPump::OnInputStreamReady [netwerk/base/nsInputStreamPump.cpp:433]
[task 2017-02-01T06:22:20.081167Z] 06:22:20     INFO - 
[task 2017-02-01T06:22:20.082655Z] 06:22:20     INFO - #28: nsInputStreamReadyEvent::Run [xpcom/io/nsStreamUtils.cpp:96]
[task 2017-02-01T06:22:20.084167Z] 06:22:20     INFO - 
[task 2017-02-01T06:22:20.084668Z] 06:22:20     INFO - #29: nsThread::ProcessNextEvent [mfbt/Maybe.h:439]
[task 2017-02-01T06:22:20.084995Z] 06:22:20     INFO - 
[task 2017-02-01T06:22:20.085764Z] 06:22:20     INFO - #30: NS_ProcessNextEvent [xpcom/threads/nsThreadUtils.cpp:389]
[task 2017-02-01T06:22:20.086215Z] 06:22:20     INFO - 
[task 2017-02-01T06:22:20.086577Z] 06:22:20     INFO - #31: mozilla::ipc::MessagePump::Run [ipc/glue/MessagePump.cpp:97]
[task 2017-02-01T06:22:20.087067Z] 06:22:20     INFO - 
[task 2017-02-01T06:22:20.087582Z] 06:22:20     INFO - #32: MessageLoop::RunInternal [ipc/chromium/src/base/message_loop.cc:238]
[task 2017-02-01T06:22:20.087855Z] 06:22:20     INFO - 
[task 2017-02-01T06:22:20.088619Z] 06:22:20     INFO - #33: MessageLoop::Run [ipc/chromium/src/base/message_loop.cc:502]
[task 2017-02-01T06:22:20.089226Z] 06:22:20     INFO - 
[task 2017-02-01T06:22:20.089784Z] 06:22:20     INFO - #34: nsBaseAppShell::Run [widget/nsBaseAppShell.cpp:158]
[task 2017-02-01T06:22:20.089834Z] 06:22:20     INFO - 
[task 2017-02-01T06:22:20.090409Z] 06:22:20     INFO - #35: nsAppStartup::Run [toolkit/components/startup/nsAppStartup.cpp:284]
[task 2017-02-01T06:22:20.090660Z] 06:22:20     INFO - 
[task 2017-02-01T06:22:20.091278Z] 06:22:20     INFO - #36: XREMain::XRE_mainRun [toolkit/xre/nsAppRunner.cpp:4462]
[task 2017-02-01T06:22:20.091777Z] 06:22:20     INFO - 
[task 2017-02-01T06:22:20.092042Z] 06:22:20     INFO - #37: XREMain::XRE_main [toolkit/xre/nsAppRunner.cpp:4638]
[task 2017-02-01T06:22:20.092536Z] 06:22:20     INFO - 
[task 2017-02-01T06:22:20.092891Z] 06:22:20     INFO - #38: XRE_main [toolkit/xre/nsAppRunner.cpp:4730]
[task 2017-02-01T06:22:20.093378Z] 06:22:20     INFO - 
[task 2017-02-01T06:22:20.093962Z] 06:22:20     INFO - #39: do_main [browser/app/nsBrowserApp.cpp:234]
[task 2017-02-01T06:22:20.094013Z] 06:22:20     INFO - 
[task 2017-02-01T06:22:20.094540Z] 06:22:20     INFO - #40: main [browser/app/nsBrowserApp.cpp:307]
[task 2017-02-01T06:22:20.095448Z] 06:22:20     INFO - 

The comment here is clearly not true any more <http://searchfox.org/mozilla-central/rev/d20e4431d0cf40311afa797868bc5c58c54790a2/dom/base/DOMParser.cpp#485>.  Nothing about setting the base URI looks at the principal as far as I can tell.  I tried passing mPrincipal to NS_NewDOMDocument and it bypasses the crash and the test passes.  I'm now testing more to see if that is a good fix.
Er, yeah.  That comment hasn't been true since the fix for bug 515401.  It's only been 7 years... ;)
(Assignee)

Comment 2

10 months ago
Yeah the local tests that I ran all passed, and the code looks pretty straightforward.  I've pushed my patches to try and will submit them for review in the mean time as well.
(Assignee)

Comment 3

10 months ago
Created attachment 8833073 [details] [diff] [review]
Part 1: Set the right principal on the documents created by DOMParser from the beginning
Attachment #8833073 - Flags: review?(bzbarsky)
(Assignee)

Comment 4

10 months ago
Created attachment 8833074 [details] [diff] [review]
Part 2: Remove DOMParser::mOriginalPrincipal
Attachment #8833074 - Flags: review?(bzbarsky)
Comment on attachment 8833073 [details] [diff] [review]
Part 1: Set the right principal on the documents created by DOMParser from the beginning

r=me
Attachment #8833073 - Flags: review?(bzbarsky) → review+
Comment on attachment 8833074 [details] [diff] [review]
Part 2: Remove DOMParser::mOriginalPrincipal

r=me
Attachment #8833074 - Flags: review?(bzbarsky) → review+

Comment 7

10 months ago
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/45b9f4c281e6
Part 1: Set the right principal on the documents created by DOMParser from the beginning; r=bzbarsky
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1fed2fda52d
Part 2: Remove DOMParser::mOriginalPrincipal; r=bzbarsky

Comment 8

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/45b9f4c281e6
https://hg.mozilla.org/mozilla-central/rev/d1fed2fda52d
Status: NEW → RESOLVED
Last Resolved: 10 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Shoud mOriginalPrincipleWasSystem also be initialised in
explicit DOMParser(nsISupports* aOwner) ?  Uses of undefined
mOriginalWasPrinciple are dynamically observable.
(In reply to Julian Seward [:jseward] from comment #9)
> Uses of undefined mOriginalWasPrinciple are dynamically observable.

Duh.  I meant "of undefined mOriginalPrincipleWasSystem".
Yes, it should!

We should really just use C++11 member initializers for things like this, imo.  It would prevent problems like that from happening.
Flags: needinfo?(ehsan)

Updated

10 months ago
Depends on: 1338156
(Assignee)

Comment 12

10 months ago
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #11)
> Yes, it should!

Yeah sorry I missed that.

> We should really just use C++11 member initializers for things like this,
> imo.  It would prevent problems like that from happening.

See bug 525063.
Flags: needinfo?(ehsan)
You need to log in before you can comment on or make changes to this bug.