717566 - head.js should use XHR instead of nsIDOMParser::parseFromStream

Status: RESOLVED FIXED

(Testing :: XPCShell Harness, defect)

Description

[Henri Sivonen (:hsivonen)]

Please migrate away from nsIDOMParser::parserFromStream so that the API can be removed.

Comment 1

[Henri Sivonen (:hsivonen)]

You can get an nsIURI from nsILocalFile, then get the string spec from the nsIURI and then pass the string URL to XMLHttpRequest.

Comment 2

[Henri Sivonen (:hsivonen)]

Also content/test/unit/head_content.js

Comment 3

[(not currently active) Ted Mielczarek]

We can use sync XHR in these cases, right? As long as we don't have to rewrite the API to be async this seems simple enough.

Comment 4

[Henri Sivonen (:hsivonen)]

(In reply to Ted Mielczarek [:ted, :luser] from comment #3)
> We can use sync XHR in these cases, right? As long as we don't have to
> rewrite the API to be async this seems simple enough.

Currently, yes.

smaug, any plans to ban sync XHR with chrome privileges to make it harder to write un-Snappy extensions?

I guess even if such a ban was put in place, we could make an exception for test harnesses.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Ah, good idea. Sync XHR should be banned in chrome at some point (in window context).
But so far, haven't heard of any plans.

Comment 6

[Raymond Lee [:raymondlee]]

Do we still want to fix this bug?  Is it ok to use sync XHR in the test harnesses?

Comment 7

[Henri Sivonen (:hsivonen)]

(In reply to Raymond Lee [:raymondlee] from comment #6)
> Do we still want to fix this bug? 

Yes.

> Is it ok to use sync XHR in the test harnesses?

That's a question for smaug.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Hmm, so where is parseFromStream used? If chrome JS only, I think that is ok.
We don't expose it to content JS anymore, since we're using
webidl bindings for DOMParser
http://mxr.mozilla.org/mozilla-central/source/dom/webidl/DOMParser.webidl?force=1

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Though, parseFromString is bad API. but so is sync XHR.

The best option would be to use *async* XHR.

Comment 10

[Thomas Wisniewski]

Attachment: 717566-remove-use-of-parseFromStream-from-head.js.diff

Here's a patch, if we still want to do this. A try run seems fine: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a1e18a08c4f7&selectedJob=26082554

Comment 11

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8783228 [details] [diff] [review]
717566-remove-use-of-parseFromStream-from-head.js.diff

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

a couple of questions.

::: dom/base/test/unit/test_xmlserializer.js
@@ +72,3 @@
>  
> +function run_namespace_tests(doc) {
> +    const de = Components.interfaces.nsIDocumentEncoder;

where is this new const 'de' used?

::: mobile/android/tests/browser/robocop/robocop_head.js
@@ -658,5 @@
> -  parser = null;
> -  stream = null;
> -  lf = null;
> -  return doc;
> -}

why do we need to edit the robocop tests?

Comment 12

[Thomas Wisniewski]

>where is this new const 'de' used?

In the encoder.init lines nearer the bottom of the function. I could declare it only once in the outer namespace, if that's cleaner.


>why do we need to edit the robocop tests?

We don't, but it's an unused function and the overall aim of these bugs was to get rid of parseFromStream entirely, so I figured it was worth removing it from the other file with head.js in its name. But I don't mind leaving it alone or making a new bug just for getting rid of it.

Comment 13

[Joel Maher ( :jmaher ) (UTC -8)]

ok, thanks for clarifying, let me r+

Comment 14

[Thomas Wisniewski]

Alright, thanks. Requesting check-in.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/b2947c489cc7
Remove use of parseFromStream in xpcshell head.js and android robocop_head.js. r=jmaher

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b2947c489cc7