Closed Bug 1453869 Opened 2 years ago Closed 2 years ago

Get rid of nsIDOMParser

Categories

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

enhancement
Not set

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)

4.83 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
5.03 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
11.78 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
12.23 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
12.25 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
2.72 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
3.17 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
8.13 KB, patch
jryans
: review+
Details | Diff | Splinter Review
45.34 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
4.54 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
11.05 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
5.28 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
5.95 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
4.19 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
3.22 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
11.24 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
No description provided.
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&regexp=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.
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().
MozReview-Commit-ID: 1fWzq6rMWf1
Attachment #8968729 - Flags: review?(mrbkap)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
MozReview-Commit-ID: B3HVscqYE6G
Attachment #8968730 - Flags: review?(mrbkap)
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)
MozReview-Commit-ID: CoepOZNb0DU
Attachment #8968732 - Flags: review?(mrbkap)
MozReview-Commit-ID: 4KuM0HRI2BC
Attachment #8968733 - Flags: review?(mrbkap)
MozReview-Commit-ID: L2QKWgDE6UM
Attachment #8968734 - Flags: review?(mrbkap)
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)
MozReview-Commit-ID: 36Op1KdLZJe
Attachment #8968736 - Flags: review?(mrbkap)
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)
MozReview-Commit-ID: A0bHF5RHhs6
Attachment #8968738 - Flags: review?(mrbkap)
The other caller went away all the way back in part 1 of bug 1453869.

MozReview-Commit-ID: pJttjegpBm
Attachment #8968739 - Flags: review?(mrbkap)
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)
Incidentally, we can remove mScriptHandlingObject, because mOwner is always the same object anyway.

MozReview-Commit-ID: 1txkjkKvBsi
Attachment #8968741 - Flags: review?(mrbkap)
Init() is no longer needed, and the mAttemptedInit machinery can all go away.

MozReview-Commit-ID: 3wNavKLGdlc
Attachment #8968742 - Flags: review?(mrbkap)
MozReview-Commit-ID: 8gGb1qO56gu
Attachment #8968743 - Flags: review?(mrbkap)
Depends on: 1454842
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)
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+
Blocks: 1455026
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)
MozReview-Commit-ID: CoepOZNb0DU
Attachment #8969165 - Flags: review?(mrbkap)
Attachment #8968732 - Attachment is obsolete: true
Attachment #8968732 - Flags: review?(mrbkap)
I've started reviewing these. I'll finish tomorrow.
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+
Attachment #8968730 - Flags: review?(mrbkap) → review+
Attachment #8968731 - Flags: review?(mrbkap) → review+
Attachment #8968733 - Flags: review?(mrbkap) → review+
Attachment #8968734 - Flags: review?(mrbkap) → review+
Attachment #8968735 - Flags: review?(mrbkap) → review+
Attachment #8968736 - Flags: review?(mrbkap) → review+
Attachment #8968738 - Flags: review?(mrbkap) → review+
Attachment #8968739 - Flags: review?(mrbkap) → review+
Attachment #8968740 - Flags: review?(mrbkap) → review+
Attachment #8968741 - Flags: review?(mrbkap) → review+
Attachment #8968742 - Flags: review?(mrbkap) → review+
Attachment #8968743 - Flags: review?(mrbkap) → review+
Attachment #8969165 - Flags: review?(mrbkap) → review+
Attachment #8969055 - Flags: review?(mrbkap) → review+
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...
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
(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.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.