717567 - toolkit/mozapps/extensions/nsBlocklistService.js should use XHR instead of DOMParser::parseFromStream

Status: RESOLVED WORKSFORME

(Toolkit :: Add-ons Manager, defect)

Description

[Henri Sivonen (:hsivonen)]

Please migrate nsBlocklistService to asynchronous XHR both to avoid synchronous IO and to allow us to get rid of nsIDOMParser::parseFromStream.

Comment 1

[Andres Hernandez [:andreshm]]

Attachment: Patch v1

Migrated nsIDOMParser to XHR async call.

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 650725 [details] [diff] [review]
Patch v1

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

Unfortunately, making _loadBlocklistFromFile() async involves quite a lot more than this. All its callers assume it fetches the data synchronously - including most of the public methods, which means changing most of the nsIBlocklistService interface and most uses of it.

The blocklist in general just needs a rewrite (bug 782261). There's a lot of architectural issues like this, and a lot of things we want to do that are rather difficult/awkward to do with the current code. 

And doing all the extra work of making the current blocklist code async seems like a wasted effort, when it would be thrown away in a more complete and more thought-out rewrite.  So in the mean time, I think we need to keep that method synchronous. 

However... apparently using sync XHR is worse than using parseFromStream(). AFAIK, its safe to use parseFromString() instead.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

So... sounds like this should be resolved wontfix and kept in mind when fixing bug 782261?

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Would like to get Henri to weigh in regarding how quickly he wants/needs to remove parseFromStream(). It looks like this bug isn't the only one that would have difficulty dealing with that.

Bug 782261 is going to take quite some time. Converting to use parseFromString() (which is sync and isn't being removed) seems like a reasonable short-term solution. But I'd also rather not have unnecessary code churn if parseFromStream() can't go away any time soon because a bunch of other things also depend on it.

Comment 5

[Henri Sivonen (:hsivonen)]

(In reply to Blair McBride (:Unfocused) from comment #4)
> Would like to get Henri to weigh in regarding how quickly he wants/needs to
> remove parseFromStream(). It looks like this bug isn't the only one that
> would have difficulty dealing with that.

I worry that if I say "not soon", it will get treated as "never", but in honesty, I expect that my work won't get blocked on this not having been fixed in the next 6 months. Can't guess further ahead than that.

> Bug 782261 is going to take quite some time. Converting to use
> parseFromString() (which is sync and isn't being removed) seems like a
> reasonable short-term solution.

Using that one wouldn't help you get rid of sync IO if you had to synchronously ingest a stream and convert in into a string in order to call parseFromString().

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Henri Sivonen (:hsivonen) from comment #5)
> I worry that if I say "not soon", it will get treated as "never", but in
> honesty, I expect that my work won't get blocked on this not having been
> fixed in the next 6 months. Can't guess further ahead than that.

Ok, that helps to know, thanks. In that case, I'd lean towards not fixing this for now. Even without the pending removal of parseFromStream(), that code needs rewritten anyway, so this won't be left unfixed forever. If for some reason bug 782261 isn't fixed in time, we can revisit this.

Andres: Does that seem reasonable to you?

Comment 7

[Andres Hernandez [:andreshm]]

Sure, np.

Comment 8

[Andrew Swan [:aswan]]

Gijs, did you fix this (or otherwise make it irrelevant) with your recent blocklist improvements?

Comment 9

[:Gijs (out for now; he/him)]

Yes, this is gone now. Though there's still other consumers of `parseFromStream`, unfortunately...