Closed
Bug 1453869
Opened 6 years ago
Closed 6 years ago
Get rid of nsIDOMParser
Categories
(Core :: DOM: Core & HTML, enhancement)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
(Blocks 1 open bug)
Details
Attachments
(16 files, 1 obsolete file)
No description provided.
Assignee | ||
Comment 1•6 years ago
|
||
Note to self: There are currently three ways of creating a DOMParser: 1) Using the no-arg constructor. This will use the subject principal and a baseURI and documentURI derived from the constructor's window, if any. But then if the subject principal is system that will get replaced with a nullprincipal. 2) Using the 3-arg constructor. According to https://searchfox.org/mozilla-central/search?q=DOMParser%5C(%5B%5E)%5D&case=true®exp=true&path= this is unused. 3) Using the contract. This gives the DOMParser a nullprincipal and a URI derived from that principal. A DOMParser can also be initialized with explicit principals by calling init() on it. This used to be noscript and we have no C++ callers outside DOMParser itself. But the webidl conversion made it scriptable... That said, I expect this method is not used for script (and will verify that via trypush). At the moment, there are only two uses of the DOMParser no-arg constructor that are called with system principals in any of the code exercised by our tests (verified via trypush). Those are in: * dom/base/test/chrome/test_bug429785.xul * dom/base/test/chrome/test_domparsing.xul Therefore, for the no-arg constructor case, I am going to just use the subject principal unless it's system, then use nullprincipal with document URI derived from it and no base URI. This will let me convert createInstance calls to the constructor, without changing their behavior. I will remove the 3-arg constructor and unused Init bits and simplify DOMParser initialization. After that it's just a matter of making it possible to import DOMParser into random globals and converting from the contract to the constructor.
Assignee | ||
Comment 2•6 years ago
|
||
OK, looks like we now do have some callers of the scripted init(). Specifically: * toolkit/modules/tests/modules/MockDocument.jsm * dom/base/test/unit/head_xml.js * dom/base/test/unit/test_xhr_document.js * dom/base/test/unit/test_cancelPrefetch.js That second one is a bit of a problem, because taking out the init() makes the parser unable to parse XUL bits. But that can dealt with by setting mOriginalPrincipalWasSystem, possibly via an explicit chromeonly thing of some sort. And nothing except xpcshell tests calls init(), as far as our tests can tell by adding MOZ_CRASH inside the WebIDL init().
Assignee | ||
Comment 3•6 years ago
|
||
MozReview-Commit-ID: 1fWzq6rMWf1
Attachment #8968729 -
Flags: review?(mrbkap)
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•6 years ago
|
||
MozReview-Commit-ID: B3HVscqYE6G
Attachment #8968730 -
Flags: review?(mrbkap)
Assignee | ||
Comment 5•6 years ago
|
||
Some DOM unit tests rely on being able to parse XUL via DOMParser. That was allowed due to them calling init() with a system subject principal. It can be more narrowly allowed by adding an explicit setter for being able to parse XUL/XBL. MozReview-Commit-ID: 3h0WWGHmYOn
Attachment #8968731 -
Flags: review?(mrbkap)
Assignee | ||
Comment 6•6 years ago
|
||
MozReview-Commit-ID: CoepOZNb0DU
Attachment #8968732 -
Flags: review?(mrbkap)
Assignee | ||
Comment 7•6 years ago
|
||
MozReview-Commit-ID: 4KuM0HRI2BC
Attachment #8968733 -
Flags: review?(mrbkap)
Assignee | ||
Comment 8•6 years ago
|
||
MozReview-Commit-ID: L2QKWgDE6UM
Attachment #8968734 -
Flags: review?(mrbkap)
Assignee | ||
Comment 9•6 years ago
|
||
In our test suite, we only run into two calls to this constructor with a system principal, and both are in test code. After this, calling the WebIDL constructor from system code is _almost_ equivalent to creating by contract. The one difference is that the resulting DOMParser (and the documents it creates) will have its script handling object set to the global the constructor came from instead of being null. MozReview-Commit-ID: Fe2yMeqoYnB
Attachment #8968735 -
Flags: review?(mrbkap)
Assignee | ||
Comment 10•6 years ago
|
||
MozReview-Commit-ID: 36Op1KdLZJe
Attachment #8968736 -
Flags: review?(mrbkap)
Assignee | ||
Comment 11•6 years ago
|
||
There are some extra hoops here because devtools has a lint to prevent Cu.importGlobalProperties, which is the normal way one would import a WebIDL constructor. MozReview-Commit-ID: 2mdNI6N1z5B
Attachment #8968737 -
Flags: review?(ttromey)
Assignee | ||
Comment 12•6 years ago
|
||
MozReview-Commit-ID: A0bHF5RHhs6
Attachment #8968738 -
Flags: review?(mrbkap)
Assignee | ||
Comment 13•6 years ago
|
||
The other caller went away all the way back in part 1 of bug 1453869. MozReview-Commit-ID: pJttjegpBm
Attachment #8968739 -
Flags: review?(mrbkap)
Assignee | ||
Comment 14•6 years ago
|
||
We always have one now. So we can remove all the codepaths that attempted to handle the !mPrincipal case. We can also remove the nsContentUtils::IsSystemPrincipal(mPrincipal) codepaths, because that can never happen: DOMParser::Constructor never creates a DOMParser with a system principal. MozReview-Commit-ID: EUrGoiI0o3u
Attachment #8968740 -
Flags: review?(mrbkap)
Assignee | ||
Comment 15•6 years ago
|
||
Incidentally, we can remove mScriptHandlingObject, because mOwner is always the same object anyway. MozReview-Commit-ID: 1txkjkKvBsi
Attachment #8968741 -
Flags: review?(mrbkap)
Assignee | ||
Comment 16•6 years ago
|
||
Init() is no longer needed, and the mAttemptedInit machinery can all go away. MozReview-Commit-ID: 3wNavKLGdlc
Attachment #8968742 -
Flags: review?(mrbkap)
Assignee | ||
Comment 17•6 years ago
|
||
MozReview-Commit-ID: 8gGb1qO56gu
Attachment #8968743 -
Flags: review?(mrbkap)
Comment 18•6 years ago
|
||
Comment on attachment 8968737 [details] [diff] [review] part 9. Stop creating DOMParser by contract in devtools I'm not that active in devtools now and I think someone else should review this.
Attachment #8968737 -
Flags: review?(ttromey) → review?(jryans)
Assignee | ||
Comment 19•6 years ago
|
||
OK. To be clear, I asked you because you were the author of the lint in question and I figured you would know whether the things I am doing to work around it are reasonable or not...
Comment on attachment 8968737 [details] [diff] [review] part 9. Stop creating DOMParser by contract in devtools Review of attachment 8968737 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, this seems reasonable to me!
Attachment #8968737 -
Flags: review?(jryans) → review+
Assignee | ||
Comment 21•6 years ago
|
||
Thunderbird uses DOMParser from C++ for now. They should ideally migrate that into JS, but we can give them something that works for the moment. MozReview-Commit-ID: C4D6QuFdbn8
Attachment #8969055 -
Flags: review?(mrbkap)
Assignee | ||
Comment 22•6 years ago
|
||
MozReview-Commit-ID: CoepOZNb0DU
Attachment #8969165 -
Flags: review?(mrbkap)
Assignee | ||
Updated•6 years ago
|
Attachment #8968732 -
Attachment is obsolete: true
Attachment #8968732 -
Flags: review?(mrbkap)
Comment 23•6 years ago
|
||
I've started reviewing these. I'll finish tomorrow.
Comment 24•6 years ago
|
||
Comment on attachment 8968729 [details] [diff] [review] part 1. Remove the unused 3-arg constructor from DOMParser I wish there was a way to mass-mark these as r+.
Attachment #8968729 -
Flags: review?(mrbkap) → review+
Updated•6 years ago
|
Attachment #8968730 -
Flags: review?(mrbkap) → review+
Updated•6 years ago
|
Attachment #8968731 -
Flags: review?(mrbkap) → review+
Updated•6 years ago
|
Attachment #8968733 -
Flags: review?(mrbkap) → review+
Updated•6 years ago
|
Attachment #8968734 -
Flags: review?(mrbkap) → review+
Updated•6 years ago
|
Attachment #8968735 -
Flags: review?(mrbkap) → review+
Updated•6 years ago
|
Attachment #8968736 -
Flags: review?(mrbkap) → review+
Updated•6 years ago
|
Attachment #8968738 -
Flags: review?(mrbkap) → review+
Updated•6 years ago
|
Attachment #8968739 -
Flags: review?(mrbkap) → review+
Updated•6 years ago
|
Attachment #8968740 -
Flags: review?(mrbkap) → review+
Updated•6 years ago
|
Attachment #8968741 -
Flags: review?(mrbkap) → review+
Updated•6 years ago
|
Attachment #8968742 -
Flags: review?(mrbkap) → review+
Updated•6 years ago
|
Attachment #8968743 -
Flags: review?(mrbkap) → review+
Updated•6 years ago
|
Attachment #8969165 -
Flags: review?(mrbkap) → review+
Updated•6 years ago
|
Attachment #8969055 -
Flags: review?(mrbkap) → review+
Assignee | ||
Comment 25•6 years ago
|
||
Yeah, having to mark them independently is annoying. I'm not sure reviewing the whole thing as a single 95KB diff would have been less annoying...
Comment 26•6 years ago
|
||
Pushed by bzbarsky@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/2bf23b953d5b part 1. Remove the unused 3-arg constructor from DOMParser. r=mrbkap https://hg.mozilla.org/integration/mozilla-inbound/rev/5576b25f547b part 2. Get rid of the unused nsIDOMParser::Init method. r=mrbkap https://hg.mozilla.org/integration/mozilla-inbound/rev/8d5d3b66c5d4 part 3. Remove the used-only-in-tests DOMParser.init method. r=mrbkap https://hg.mozilla.org/integration/mozilla-inbound/rev/432d897ff4a9 part 4. Remove nsIDOMParser::ParseFromString. r=mrbkap https://hg.mozilla.org/integration/mozilla-inbound/rev/00340263aeb2 part 5. Remove nsIDOMParser::ParseFromBuffer. r=mrbkap https://hg.mozilla.org/integration/mozilla-inbound/rev/ba157de852ef part 6. Remove nsIDOMParser::ParseFromStream. r=mrbkap https://hg.mozilla.org/integration/mozilla-inbound/rev/1ad69430054b part 7. Make the DOMParser WebIDL constructor use a nullprincipal for the document if the caller is system-principal. r=mrbkap https://hg.mozilla.org/integration/mozilla-inbound/rev/72056af41ae0 part 8. Teach Cu.importGlobalProperties about DOMParser. r=mrbkap https://hg.mozilla.org/integration/mozilla-inbound/rev/7ff0d0f4e9ab part 9. Stop creating DOMParser by contract in devtools. r=tromey https://hg.mozilla.org/integration/mozilla-inbound/rev/0eb930d13979 part 10. Stop constructing DOMParser by contract. r=mrbkap https://hg.mozilla.org/integration/mozilla-inbound/rev/4becf7582577 part 11. Inline DOMParser::InitInternal into its one caller. r=mrbkap https://hg.mozilla.org/integration/mozilla-inbound/rev/4ff1ac9af38f part 12. Make the DOMParser constructor require a principal. r=mrbkap https://hg.mozilla.org/integration/mozilla-inbound/rev/07be8e0c8d40 part 13. Make DOMParser store an nsIGlobalObject* as mOwner. r=mrbkap https://hg.mozilla.org/integration/mozilla-inbound/rev/697cfb2ce42d part 14. Make the document and base URIs arguments to the DOMParser constructor. r=mrbkap https://hg.mozilla.org/integration/mozilla-inbound/rev/3c6dbba97910 part 15. Remove nsIDOMParser. r=mrbkap https://hg.mozilla.org/integration/mozilla-inbound/rev/757c523ca40f part 16. Add a way to create a DOMParser directly from C++. r=mrbkap
Comment 27•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2bf23b953d5b https://hg.mozilla.org/mozilla-central/rev/5576b25f547b https://hg.mozilla.org/mozilla-central/rev/8d5d3b66c5d4 https://hg.mozilla.org/mozilla-central/rev/432d897ff4a9 https://hg.mozilla.org/mozilla-central/rev/00340263aeb2 https://hg.mozilla.org/mozilla-central/rev/ba157de852ef https://hg.mozilla.org/mozilla-central/rev/1ad69430054b https://hg.mozilla.org/mozilla-central/rev/72056af41ae0 https://hg.mozilla.org/mozilla-central/rev/7ff0d0f4e9ab https://hg.mozilla.org/mozilla-central/rev/0eb930d13979 https://hg.mozilla.org/mozilla-central/rev/4becf7582577 https://hg.mozilla.org/mozilla-central/rev/4ff1ac9af38f https://hg.mozilla.org/mozilla-central/rev/07be8e0c8d40 https://hg.mozilla.org/mozilla-central/rev/697cfb2ce42d https://hg.mozilla.org/mozilla-central/rev/3c6dbba97910 https://hg.mozilla.org/mozilla-central/rev/757c523ca40f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment 28•6 years ago
|
||
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #25) > Yeah, having to mark them independently is annoying. I'm not sure reviewing > the whole thing as a single 95KB diff would have been less annoying... Nope, this is much easier to review, just more annoying to stamp.
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•