1454842 - Port bug 1453869: Replace use of nsIDOMParser

Status: RESOLVED FIXED

(Thunderbird :: General, enhancement)

Description

[Boris Zbarsky [:bzbarsky]]

See bug 1453869.

The following changes will need to be made:

1) In JS, stop creating DOMParser by contract.  Just Cu.importGlobalProperties(["DOMParser"]) if you're not in a window scope.  After that, use |new DOMParser()|.  This affects a few callsites in chat/ and mail/ and is easy to deal with.

2) In C++, stop creating DOMParser by contract.  This affects a single caller: nsWMUtils::MakeXMLdoc.  This might be a problem, because after bug 1453869 there is no way to create a DOMParser that is not associated with an nsIGlobalObject.  And in particular there is no mechanism to really create one from non-script.  If needed, I could probably add such a mechanism, but it would require some thought in terms of how it should actually behave.  Most simply, we could have something that has a null owner, a nullprincipal for the principal, a document URI from that nullprincipal, no base URI...

That's assuming nsWMUtils::MakeXMLdoc isn't dead code to start with.

Comment 1

[Jorg K (CEST = GMT+2)]

Thanks for the heads-up.

I looked at the C++ call site. This is part of the "Windows Live Mail" import. That client was created for Windows 7, but is still compatible with Windows 10 (https://en.wikipedia.org/wiki/Windows_Live_Mail). It stores its configuration data in XML files, see bug 394687 comment #32. I've never used WLM nor do I know whether the import still works, but as far as I can see, it's not dead code and it would also not be a good idea to kill WLM import completely (as we've done with Eudora).

I'm sure you've read the code around
https://dxr.mozilla.org/comm-central/rev/304145e6e1ce10ddab2fc911a1a98e397cab816e/mailnews/import/winlivemail/nsWMUtils.cpp#143
So we create the parser and then use parser->ParseFromStream() to parse a stream that comes from a file.

Later the XML doc is used in mailnews/import/winlivemail/nsWMSettings.cpp where we extract values from it, like:
  nsWMUtils::GetValueForTag(xmlDoc, "Expire_Days", value)

nsWMUtils::GetValueForTag() can be found here:
https://dxr.mozilla.org/comm-central/rev/304145e6e1ce10ddab2fc911a1a98e397cab816e/mailnews/import/winlivemail/nsWMUtils.cpp#153

So if you have a better idea of how to parse an XML file and get values from it, please let me know and we're happy to switch to another method.

Comment 2

[Boris Zbarsky [:bzbarsky]]

I would somewhat appreciate it if you did not copy existing bug summaries into the summary of these "Port *" bugs.  It makes the bugmail _really_ confusing and hard to skim...

For the rest, I think there are two main options:

1)  I can add a C++ DOMParser::ConstructWithoutGlobal() method on top of the current patches for bug 1453869.
2)  Some of the work of nsWMUtils could be moved to a JSM.

The latter would be more future-proof, because the former might be liable to get removed if someone forgets to check for comm-central consumers.

Comment 3

[Jorg K (CEST = GMT+2)]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #2)
> I would somewhat appreciate it if you did not copy existing bug summaries
> into the summary of these "Port *" bugs.  It makes the bugmail _really_
> confusing and hard to skim...
Hmm, well, our practise is that when we have M-C "Do such and such" we usually create a bug for C-C like "Port |Bug xx - Do such and such|", or a variation thereof. That gives us the information what's going on in M-C and the bug number, so we can get there quickly. We also skip "Port *" when looking for candidates for release notes.

In this case the original summary is "Get rid of nsIDOMParser" and since you filed this bug here, that summary wasn't copied so we have "Port bug 1453869: Update to removal of nsIDOMParser". Is that still not acceptable? How does that make it confusing? Wouldn't you just skip the "Port *" bugs? Do you have a better suggestion? I'm afraid to do what I always do with bug 1455055 now :-(

> For the rest, I think there are two main options:
> 1)  I can add a C++ DOMParser::ConstructWithoutGlobal() method on top of the
> current patches for bug 1453869.
> 2)  Some of the work of nsWMUtils could be moved to a JSM.
I'd rather have a C++ replacement so we have a three line change instead of redoing parts in JS and then not know how to test it. Surely a comment would tell future generations that this is used in C-C. Besides, there is C++ under the covers, no?

Comment 4

[Boris Zbarsky [:bzbarsky]]

> our practise is

I'm aware of what your practice is.  I'm saying it screws up my bugmail.  ;)  If you have to do it, ok, but if there's a way to not copy the exact summary in its entirety, I would appreciate it.

> Wouldn't you just skip the "Port *" bugs?

They basically take up mental energy to classify and delete.  Not all "Port *" bugs are yours, so I can't just auto-delete them.

> I'd rather have a C++ replacement

Just as long as you're clear that it can break silently at any time.  And by "silently" I mean it might keep compiling but stop working correctly...

Comment 5

[Jorg K (CEST = GMT+2)]

Attachment: 1454842-nsIDOMParser.patch - JS part

I hope the summary is better now. I'm happy not to copy the M-C bug verbatim but instead express what we need to do ... if I know. I'm still not sure what that would be in case of bug 1455076.

Here's the JS part. Can this land now or do we have to wait for the parser to become available via Cu.importGlobalProperties() like in the other bug for the XMLSerializer?

Comment 6

[Boris Zbarsky [:bzbarsky]]

> I'm still not sure what that would be in case of bug 1455076

Change HandleEvent(nsIDOMEvent*) to HandleEvent(mozilla::dom::Event*), more or less.

> Can this land now or do we have to wait for the parser to become available via Cu.importGlobalProperties()

The latter.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8969039 [details] [diff] [review]
1454842-nsIDOMParser.patch - JS part

r=me

Comment 8

[Jorg K (CEST = GMT+2)]

Comment on attachment 8969039 [details] [diff] [review]
1454842-nsIDOMParser.patch - JS part

Philipp, would you like to review the Calendar hunks?

Comment 9

[Boris Zbarsky [:bzbarsky]]

I posted a patch for a thing you can use for the C++ use at <https://bug1453869.bmoattachments.org/attachment.cgi?id=8969055>.  It can be used like so:

  ErrorResult rv;
  RefPtr<DOMParser> parser = DOMParser::CreateWithoutGlobal(rv);
  if (rv.Failed()) {
    // handle that however you want
  }

Comment 10

[Jorg K (CEST = GMT+2)]

Thanks, Boris.

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #4)
> Just as long as you're clear that it can break silently at any time.  And by
> "silently" I mean it might keep compiling but stop working correctly...
Surely a simple Xpcshell test would prevent that, no? Open a pre-canned XML file, and get a value for tag. That's what we do.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> Surely a simple Xpcshell test would prevent that, no?

No, because that will be running the script-created version.  The script-created version will obviously continue to work, since it's used all over and tested extensively.  The CreateWithoutGlobal thing will not be used or tested by anything outside Thunderbird.

Comment 12

[Jorg K (CEST = GMT+2)]

Sorry, of course. How about a gtest then? That can exercise C++.

Comment 13

[Boris Zbarsky [:bzbarsky]]

If you want to write one for Thunderbird, please feel free.  I'm not going to spend more time on maintaining this codepath than I already have....

Comment 14

[Jorg K (CEST = GMT+2)]

Attachment: 1454842-nsIDOMParser-cpp.patch

This has become a far bigger change than comment #9 suggests ;-(

Since I will land this now, an r- wouldn't be useful, but I promise to follow up on anything you don't like :-)

+  nsCOMPtr<nsIDocument> xmldoc =
+    parser->ParseFromStream(stream, EmptyString(), int32_t(filesize),
+                            mozilla::dom::SupportedType::Application_xml, rv2);
+  xmldoc.forget(aXmlDoc);
+  return rv2.StealNSResult();

This is OK, no? - Even if xmldoc is null?

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/b398644acb93
Port bug 1453869: Replace use of nsIDOMParser (JS part). r=bz
https://hg.mozilla.org/comm-central/rev/e55693abfce0
Port bug 1453869: Replace use of nsIDOMParser (C++ part). rs=bustage-fix

Comment 16

[Jorg K (CEST = GMT+2)]

Sadly this gave a heap of test failures:

Xpcshell:
=========
TEST-UNEXPECTED-FAIL | xpcshell-libical.ini:comm/calendar/test/unit/test_alarmutils.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | xpcshell-libical.ini:comm/calendar/test/unit/test_ltninvitationutils.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | comm/mailnews/base/test/unit/test_autoconfigXML.js | xpcshell return code: 0

The last one is an obvious omission, patch coming.

test_alarmutils.js says:
 0:11.93 ERROR Unexpected exception NS_ERROR_NOT_AVAILABLE:
createOwnedXULNode@resource://calendar/modules/utils/calAlarmUtils.jsm:140:20
setupActionImage@resource://calendar/modules/utils/calAlarmUtils.jsm:144:33
addReminderImages@resource://calendar/modules/utils/calAlarmUtils.jsm:166:38
test_addReminderImages@c:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/_tests/xpcshell/comm/calendar/test/unit/test_alarmutils.js:221:5
async*run_next_test/_run_next_test/<@c:\mozilla-source\comm-central\testing\xpcshell\head.js:1425:22
async*_run_next_test@c:\mozilla-source\comm-central\testing\xpcshell\head.js:1425:10
run@c:\mozilla-source\comm-central\testing\xpcshell\head.js:676:9
_do_main@c:\mozilla-source\comm-central\testing\xpcshell\head.js:217:3
_execute_test@c:\mozilla-source\comm-central\testing\xpcshell\head.js:521:5
@-e:1:1

 0:11.93 INFO exiting test
 0:11.93 INFO "CONSOLE_MESSAGE: (error) [JavaScript Error: "XML Parsing Error:
Location: moz-nullprincipal:{4901ace8-4646-420a-897b-614d98dbb7b1}
Line Number 1, Column 79:" {file: "moz-nullprincipal:{4901ace8-4646-420a-897b-614d98dbb7b1}" line: 1 column: 79 source: "<window xmlns='http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul'><box/></window>"}]"

which comes from this code:
        function createOwnedXULNode(elem) {
            const XUL_NS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
            return aElement.ownerDocument.createElementNS(XUL_NS, elem);
        }

Philipp, care to take a look? Boris, to the refactored parser behaves differently?

Mozmill:
========
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1524308352/build/tests/mozmill/composition/test-image-insertion-dialog.js | test-image-insertion-dialog.js::test_image_insertion_dialog_persist (never before seen on Mac)
Log: EXCEPTION: Timeout waiting for modal dialog to open.

TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1524308352/build/tests/mozmill/composition/test-multipart-related.js | test-multipart-related.js::test_basic_multipart_related
Log: EXCEPTION: a != b: 'text/html' != 'multipart/related'.

TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1524308580/build/tests/mozmill/newmailaccount/test-newmailaccount.js
Log: Heaps of EXCEPTION: Timeout waiting for modal dialog to open since heaps of subtest are failing.

These Mozmill failures might need to go into another bug. Aceman, care to take a look?

Comment 17

[Jorg K (CEST = GMT+2)]

Attachment: 1454842-follow-up-js.patch - JS follow-up

Comment 18

[Jorg K (CEST = GMT+2)]

Just for the record, all those tests passed before, so here the regression interval
M-C last good: 39ccabfd7d0712a45335325cb24b0e0b2b
M-C first bad: dd0e54d786743974a50a338059bcd68a09
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=39ccabfd7d0712a45335325cb24b0e0b2b&tochange=dd0e54d786743974a50a338059bcd68a09
which is the merge this morning.

Comment 19

[:aceman]

Comment on attachment 8969039 [details] [diff] [review]
1454842-nsIDOMParser.patch - JS part

Review of attachment 8969039 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/components/newmailaccount/content/uriListener.js
@@ +11,5 @@
>  ChromeUtils.import("resource://gre/modules/Services.jsm");
>  ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
>  ChromeUtils.import("resource://gre/modules/NetUtil.jsm");
>  ChromeUtils.import("resource:///modules/JXON.js");
> +Cu.importGlobalProperties(["DOMParser"]);

This one seems to cause NS_ERROR_NOT_AVAILABLE: Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIXPCComponents_Utils.importGlobalProperties] uriListener.js:15 at TB startup.

What is it missing?

Comment 20

[Jorg K (CEST = GMT+2)]

Comment on attachment 8969890 [details] [diff] [review]
1454842-follow-up-js.patch - JS follow-up

BTW, the init method was removed here
https://hg.mozilla.org/mozilla-central/rev/5576b25f547b#l3.34
and there are no calls of parseString() passing the URIs, to that's safe to remove.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/e6604746c70e
Port bug 1453869: Replace use of nsIDOMParser (JS part, take 2). rs=bustage-fix

Comment 22

[Jorg K (CEST = GMT+2)]

OK, test_ltninvitationutils.js passes now, so fixed by the init removal.

We're left with:
Xpcshell: test_alarmutils.js, XML parsing error, see comment #16.
Mozmill: test-image-insertion-dialog.js, test-multipart-related.js and test-newmailaccount.js, see comment #19 for the latter.
I'll take a look at test-multipart-related.js and maybe transfer this to a separate bug.

Comment 23

[Jorg K (CEST = GMT+2)]

Filed bug 1455903 for test-image-insertion-dialog.js, test-multipart-related.js.

So let's concentrate on test_alarmutils.js, XML parsing error, see comment #16 and test-newmailaccount.js, see comment #19 here.

Comment 24

[Jorg K (CEST = GMT+2)]

Attachment: 1454842-follow-up-js2.patch - another JS follow-up

Removing
Cu.importGlobalProperties(["DOMParser"])
makes the account creation test pass.

So we're down to one failing test:
test_alarmutils.js, XML parsing error, see comment #16
Line Number 1, Column 79:" {file: "moz-nullprincipal:{4901ace8-4646-420a-897b-614d98dbb7b1}" line: 1 column: 79 source: "<window xmlns='http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul'><box/></window>"}]"

If you count the characters, 79 is right before <box/>.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/d22214f79166
Port bug 1453869: Replace use of nsIDOMParser (JS part, take 3). rs=bustage-fix

Comment 26

[:aceman]

Comment on attachment 8969954 [details] [diff] [review]
1454842-follow-up-js2.patch - another JS follow-up

Review of attachment 8969954 [details] [diff] [review]:
-----------------------------------------------------------------

But why is this needed? Both files do use DOMParser so why is it better to remove the import of it?

Comment 27

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8969880 [details] [diff] [review]
1454842-nsIDOMParser-cpp.patch

>+LOCAL_INCLUDES += [

You shouldn't need that.

I didn't carefully double-check the nsIDOMDocument/nsIDocument bits.

>+#include "DOMParser.h"  // From mozilla/dom/base via moz.build's LOCAL_INCLUDES.

Include "mozilla/dom/DOMParser.h", which is where the header is exported.  Why not?

>+++ b/mailnews/import/winlivemail/nsWMUtils.h
>-class nsIDOMDocument;
>+#include "nsIDocument.h"

I doubt you need this change.

Comment 28

[Boris Zbarsky [:bzbarsky]]

> This one seems to cause NS_ERROR_NOT_AVAILABLE

Sounds like that code is running in a Window scope, where DOMParser already exists.

> Line Number 1, Column 79:" {file: "moz-nullprincipal:{4901ace8-4646-420a-897b-614d98dbb7b1}" line: 1 column: 79 source: "<window xmlns='http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul'><box/></window>"}]"

Looks like that test trying to parse XUL via a DOMParser.  This used to be possible if you called init() on the parser in system code.  If you need that now, in system code, you need to call forceEnableXULXBL() on the DOMParser before parsing.

Given that calendar/test/unit/test_alarmutils.js does in fact do cal.xml.parseString() with XUL bits, and parseString used to have an init() call, that's what's going on there.  So you probably want to just forceEnableXULXBL() in parseString for now.

Comment 29

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8969890 [details] [diff] [review]
1454842-follow-up-js.patch - JS follow-up

r=me modulo comment 28.

Comment 30

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #26)
> But why is this needed? Both files do use DOMParser so why is it better to
> remove the import of it?
Good question. The Mozmill test passed with both removals, but on the other hand, some Xpcshell test fails now:
TEST-UNEXPECTED-FAIL | comm/mailnews/base/test/unit/test_autoconfigFetchDisk.js | xpcshell return code: 0
ReferenceError: DOMParser is not defined at chrome://messenger/content/accountcreation/fetchConfig.js:28

As Boris said: "code is running in a Window scope".

Comment 31

[Jorg K (CEST = GMT+2)]

Attachment: 1454842-nsIDOMParser-cpp-follow-up.patch

Adjusting the includes as pointed out by Boris in comment #27.

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/5f12a1e8143b
Port bug 1453869: Replace use of nsIDOMParser (C++ part, follow-up). r=bz DONTBUILD

Comment 33

[Jorg K (CEST = GMT+2)]

OK, adding forceEnableXULXBL() fixes the alarm test.

To the last issue is to see how to Cu.importGlobalProperties(["DOMParser"]) so the Xpcshell test test_autoconfigFetchDisk.js and the Mozmill test pass.

Comment 34

[Jorg K (CEST = GMT+2)]

Well, test_autoconfigFetchDisk.js passes with Cu.importGlobalProperties(["DOMParser"]) in fetchConfig.js and test-newmailaccount.js fails. So hard to win here :-(

Comment 35

[Jorg K (CEST = GMT+2)]

Attachment: 1454842-follow-up-js3.patch - third JS follow-up

This should now keep everyone happy :-)

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=d2bfedb6794604035bf3543dd47cc1d329b84199

Comment 36

[:aceman]

(In reply to Jorg K (GMT+1) from comment #34)
> Well, test_autoconfigFetchDisk.js passes with
> Cu.importGlobalProperties(["DOMParser"]) in fetchConfig.js and
> test-newmailaccount.js fails. So hard to win here :-(

Is it possible to detect if running in 'window scope' and do the right thing?

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/79b81526602a
Port bug 1453869: Replace use of nsIDOMParser (JS part, take 4). rs=bustage-fix

Comment 38

[Jorg K (CEST = GMT+2)]

Comment on attachment 8969954 [details] [diff] [review]
1454842-follow-up-js2.patch - another JS follow-up

This removal *was* necessary. Please open another bug for fine-tuning.

Comment 39

[Jorg K (CEST = GMT+2)]

Comment on attachment 8969977 [details] [diff] [review]
1454842-follow-up-js3.patch - third JS follow-up

Philipp for the calendar hunk.

Sorry about the mess in this bug. I had a pre-approved patch which sadly caused all sorts of test failures, hence three follow-ups for the JS part.

Under "normal" development conditions there would have been enough time for various try runs.

Comment 40

[Boris Zbarsky [:bzbarsky]]

> Is it possible to detect if running in 'window scope' and do the right thing?

  if (!this.DOMParser) {
    Cu.importGlobalProperties(["DOMParser"]);
  }

in global scope.

Comment 41

[:aceman]

Do we actually need a DOMparser? OR is it just a misleading name? If we want to read in some XML files, don't we need some XMLparser? Why is any 'window' context relevant here? Reading in xml files may be needed in any context, e.g. in backend without UI (as in xpcshell).

Comment 42

[Boris Zbarsky [:bzbarsky]]

DOMParser can parse HTML, not just XML.  But anyway, the name is moderately legacy.

> Why is any 'window' context relevant here?

It's not.  But the web platform assumes that all Document objects have some JS global they are associated with, and while Gecko doesn't 100% rely on that right now we may well end up there in the not too far-off future.  Now that JS global doesn't have to be a Window, of course (though in the web platform it always is).

Comment 43

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 8969890 [details] [diff] [review]
1454842-follow-up-js.patch - JS follow-up

Review of attachment 8969890 [details] [diff] [review]:
-----------------------------------------------------------------

If you are removing the parameters docs, you'll also need to remove them from the function signature and adapt the callers to not pass more than one argument.

Comment 44

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 8969977 [details] [diff] [review]
1454842-follow-up-js3.patch - third JS follow-up

Review of attachment 8969977 [details] [diff] [review]:
-----------------------------------------------------------------

...which I now see you are in part doing here, but callers still need to be adapted.

Comment 45

[Jorg K (CEST = GMT+2)]

Thanks, I fixed the mess-up later. I didn't see any callers which pass the URIs, although "parseString" is a very generic name used elsewhere, so I'm not 100% sure. Let me know if you can spot a call site that needs fixing.

Comment 46

[Jorg K (CEST = GMT+2)]

I think together both patches should be r+ and we're done here. What am I missing?

Comment 47

[Jorg K (CEST = GMT+2)]

Also see comment #20:
... there are no calls of parseString() passing the URIs, so that's safe to remove.

Comment 48

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 8969977 [details] [diff] [review]
1454842-follow-up-js3.patch - third JS follow-up

You are right, sorry about that. r=philipp across both patches.