1500026 - The FallbackEncoding instance is not listening to the "intl:requested-locales-changed" event

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Gabriele Svelto [:gsvelto] (PTO)]

I stumbled upon this issue while working on bug 1435683: the FallbackEncoding object is trying to register a weakly-held observer here:

https://searchfox.org/mozilla-central/rev/eef79962ba73f7759fd74da658f6e5ceae0fc730/dom/encoding/FallbackEncoding.cpp#164

However since it doesn't support the nsIWeakReference interface the call fails silently, hence the observer is never registered.

Comment 1

[Gabriele Svelto [:gsvelto] (PTO)]

This code has been added in bug 1346616 and it's always been this way, Zibi since you authored that patch is the above an issue or not? Since that observer has never been registered I was wondering what the proper course of action should be: adding it as strong reference (so that it's added properly) or just removing all that code since it was never used?

Comment 2

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH] Litsen to locale changes correctly

This is a tentative fix to solve the issue. While doing it I noticed a second issue with the code: the removal of the observer in the Shutdown() method will never be called because by that time the observer service has already been torn down. If the observer is registered with a weak reference this is obviously not a problem but it was worth mentioning.

Comment 3

[Zibi Braniecki [:zbraniecki][:gandalf]]

Thanks for poking me Gabriele. This code is related to encodings so, I'll redirect the question to Jonathan who knows this area way better :)

Comment 4

[Jonathan Kew [:jfkthame]]

I think we should fix (rather than remove) the observer code here (although in practice I expect the occasions when anyone cares about an on-the-fly change to the pref causing a change in the fallback encoding will be so rare as to be insignificant!)

For bonus points, can we have a test that changes the requested-locales pref and verifies that the expected change to the fallback encoding actually happens? Obviously we currently lack some coverage here, as we didn't realize it was broken.

Comment 5

[Gabriele Svelto [:gsvelto] (PTO)]

I can definitely fix this and maybe I can figure out how to write a test too.

Comment 6

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: Bug 1500026 - Make the fallback encoding object listen to locale changes

Comment 7

[Gabriele Svelto [:gsvelto] (PTO)]

I've poked around with the tests a bit but couldn't find a simple way to test this. How do I check which fallback encoding is in use?

Comment 8

[Jonathan Kew [:jfkthame]]

Maybe you can try loading a data: URL that contains some high-8-bit bytes with no charset attribute, and inspect the textContent of the resulting document to see how it got interpreted? I assume that depends on the fallback encoding object.

If you load something like

    data:text/html,%88%97%a6%b5%c4%d3%e2%f1

and try different options from the View / Encoding submenu, you can see how the result differs depending on the encoding used.

So I think it should work to have a test that sets various values for the requested locales, and in each case loads a document like this and checks that the expected characters end up in the DOM.

Comment 9

[Gabriele Svelto [:gsvelto] (PTO)]

Sorry for the delay but I couldn't find the spare time to write the tests here. We can land my fix as is or I can leave the bug until I have more time to write them.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1b9f8a4d684b
Make the fallback encoding object listen to locale changes r=jfkthame

Comment 11

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/1b9f8a4d684b