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

RESOLVED FIXED in Firefox 51

Status

RESOLVED FIXED
7 years ago
2 years ago

People

(Reporter: hsivonen, Unassigned)

Tracking

(Blocks: 1 bug)

unspecified
mozilla51
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

7 years ago
Please migrate away from nsIDOMParser::parserFromStream so that the API can be removed.
(Reporter)

Comment 1

7 years ago
You can get an nsIURI from nsILocalFile, then get the string spec from the nsIURI and then pass the string URL to XMLHttpRequest.
(Reporter)

Comment 2

7 years ago
Also content/test/unit/head_content.js
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.
(Reporter)

Comment 4

7 years ago
(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

7 years ago
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.
Do we still want to fix this bug?  Is it ok to use sync XHR in the test harnesses?
(Reporter)

Comment 7

6 years ago
(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.
Flags: needinfo?(bugs)

Comment 8

6 years ago
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
Flags: needinfo?(bugs)

Comment 9

6 years ago
Though, parseFromString is bad API. but so is sync XHR.

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

Comment 10

2 years ago
Created attachment 8783228 [details] [diff] [review]
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
Attachment #8783228 - Flags: review?(jmaher)
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?
Attachment #8783228 - Flags: review?(jmaher) → review-

Comment 12

2 years ago
>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.
Flags: needinfo?(jmaher)
ok, thanks for clarifying, let me r+
Flags: needinfo?(jmaher)
Attachment #8783228 - Flags: review- → review+

Comment 14

2 years ago
Alright, thanks. Requesting check-in.
Keywords: checkin-needed

Comment 15

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/b2947c489cc7
Remove use of parseFromStream in xpcshell head.js and android robocop_head.js. r=jmaher
Keywords: checkin-needed

Comment 16

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b2947c489cc7
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.