Closed Bug 1500026 Opened Last year Closed Last year

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox65 --- fixed

People

(Reporter: gsvelto, Assigned: gsvelto)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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.
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?
Flags: needinfo?(gandalf)
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.
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 :)
Flags: needinfo?(gandalf) → needinfo?(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.
Flags: needinfo?(jfkthame)
Priority: -- → P2
I can definitely fix this and maybe I can figure out how to write a test too.
Assignee: nobody → gsvelto
Status: NEW → ASSIGNED
Attachment #9018635 - Attachment is obsolete: true
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?
Flags: needinfo?(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.
Flags: needinfo?(jfkthame)
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.
Blocks: 1509298
Pushed by gsvelto@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1b9f8a4d684b
Make the fallback encoding object listen to locale changes r=jfkthame
https://hg.mozilla.org/mozilla-central/rev/1b9f8a4d684b
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.