Closed Bug 1336213 Opened 9 years ago Closed 9 years ago

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

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox54 --- fixed

People

(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)

References

Details

Attachments

(2 files)

Part 1: Set the right principal on the documents created by DOMParser from the beginning
4.99 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
Part 2: Remove DOMParser::mOriginalPrincipal
4.01 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
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... ;)
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.
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+
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
https://hg.mozilla.org/mozilla-central/rev/45b9f4c281e6 https://hg.mozilla.org/mozilla-central/rev/d1fed2fda52d
Status: NEW → RESOLVED
Closed: 9 years 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)
Depends on: 1338156
(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)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: