Enable DocumentL10n in non-system-principal

RESOLVED FIXED in Firefox 65

Status

()

P2
normal
RESOLVED FIXED
7 months ago
3 months ago

People

(Reporter: gandalf, Assigned: gandalf)

Tracking

(Blocks: 4 bugs)

unspecified
mozilla65
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox65 fixed)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Assignee)

Description

7 months ago
In bug 1455649 we enable DocumentL10n API on nsIDocument but we had to limit the API to work only with system principal because it cannot return the Promise with values from system principal to non-system principal.

This bug will be about extending the API to support that.
(Assignee)

Updated

7 months ago
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Depends on: 1455649
Priority: -- → P2
Blocks: 1494039
Please test with resource://payments/paymentRequest.xhtml to ensure that this meets the needs of web payments now that you closed bug 1407418.
This is a PoC to make document.l10n work in unprivileged documents. It works by
listening to the promise returned from the privileged code then cloning the
result into the unprivileged context and passing that to a promise generated for
the unprivileged page.
Comment on attachment 9014620 [details]
Bug 1488973: Wrap privileged promises in a promise for the document we're returning to.

This is a PoC for fixing the issue with promises. It passes tests and seems to work in the about:robots case. What do you think?
Attachment #9014620 - Flags: feedback?(gandalf)
(Assignee)

Comment 4

6 months ago
Comment on attachment 9014620 [details]
Bug 1488973: Wrap privileged promises in a promise for the document we're returning to.

Damn, that looks clean.

Thank you Dave!
Attachment #9014620 - Flags: feedback?(gandalf) → feedback+
(Assignee)

Comment 9

6 months ago
Dave, I updated your patches to:
 - skip creating the wrapper for system principal
 - only add browser's omni.ja if the app is a browser

I launched a talos test with the changes: https://treeherder.mozilla.org/#/jobs?repo=try&revision=68be1324252b4c48636f8a01a8540ce166083c65

And we should get a perfherder results soon: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=72eed19d131e&newProject=try&newRevision=68be1324252b

Dave - I could not get the test to work locally at all - I tried `./mach test intl/l10n/test` and it opens a new window for "resource://l10n-test/test.html" and an error: "Firefox can’t find the file at /home/zbraniecki/projects/mozilla-unified/obj-x86_64-pc-linux-gnu-opt/_tests/testing/mochitest/browser/intl/l10n/test.html."

Can you help me workaround it?
Flags: needinfo?(dtownsend)
(Assignee)

Comment 10

6 months ago
Hmm, my original patch had an error (B2G ID instead of Firefox ID).

Updated: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=72eed19d131e&newProject=try&newRevision=921f4ecc001a&framework=1

Some small regression in `cpstartup content-process-startup` but wins in `about_preferences_basic`.

Keeping the NI on Dave for the tests.
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #9)
> Dave - I could not get the test to work locally at all - I tried `./mach
> test intl/l10n/test` and it opens a new window for
> "resource://l10n-test/test.html" and an error: "Firefox can’t find the file
> at
> /home/zbraniecki/projects/mozilla-unified/obj-x86_64-pc-linux-gnu-opt/_tests/
> testing/mochitest/browser/intl/l10n/test.html."
> 
> Can you help me workaround it?

You need to add a "/" on the end of the uri at the top of the test file.
Flags: needinfo?(dtownsend)
(Assignee)

Updated

6 months ago
Attachment #9014620 - Attachment is obsolete: true
(Assignee)

Updated

6 months ago
Attachment #9014948 - Attachment is obsolete: true
(Assignee)

Updated

6 months ago
Attachment #9014949 - Attachment is obsolete: true
(Assignee)

Comment 13

6 months ago
Tests added!
(Assignee)

Comment 14

6 months ago
Mossop - I think the performance trade-off is your call. Are you ok landing this patchset with the tradeoff, or would you prefer us to try to optimize it?

On IRC you suggested lazily loading L10nRegistry. I'm not sure if I understand how would that work. Currently, any piece of code (in particular, process bootstrap and AddonManager) can register FileSources.

If we go the naive route and just move the L10nRegistry initialization to mozIDOMLocalization constructor which would load L10nRegistry JSM if it's not loaded, then I'm not sure how it would impact AddonsManager (and overall, will AddonsManager register langpacks per process??)

Alternatively, we could try to rewire L10nRegistry to add/remove sources and fetch/cache I/O only in parent process and create a content process to just fetch data from the parent process but then we'd have to transfer all of the FTL resources (in form of parsed JSON) from parent to content, which make make the content process localization slower. Is that the right tradeoff? And should it block this bug?
Flags: needinfo?(dtownsend)
Blocks: 1485002
(Assignee)

Comment 15

5 months ago
One more idea, if implementing a full IPC between parent/content process is going to take time, we could keep the l10nregistries separate for now, but align their sources so that only in the parent you can add/remove/update sources, and L10nRegistry constructor in the content process just looks up for the sources registered in the parent one and initializes with them.

This would allow us to initialize L10nRegistry in content process lazily.

Mossop - does it sound like something worth doing, or would you recommend going for full IPC?
(Assignee)

Comment 16

5 months ago
Ok, I went for the idea from comment 15. I used pmm.sharedData to allow for lazy loading of L10nRegistry in content processes.

I kept the source registration in nsBrowserGlue to keep it per-product and only in parent process.

Here's new talos: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=c2c98b5b994d&newProject=try&newRevision=d907ff4c6b08&framework=1

I still think we need to go for the good IPC solution, but we should be able to land this right as 65 open without having to wait for bug 1462841.
Flags: needinfo?(dtownsend)
(Assignee)

Updated

5 months ago
Target Milestone: --- → mozilla65

Comment 18

5 months ago
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/819df51e3083
Wrap privileged promises in a promise for the document we`re returning to. r=smaug
https://hg.mozilla.org/integration/autoland/rev/c06b2c594294
Register locale sources in all processes. r=mossop
https://hg.mozilla.org/integration/autoland/rev/6eab8a1b17ce
Add tests for localisation of unprivileged content. r=flod,mossop

Comment 19

5 months ago
Backout by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/deb666fd309c
Backed out 3 changesets for causing TV failures on browser_resource_uri.js
:gandalf , there were also xpcshell failures on test_webextension_langpack.js

Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=207452385&repo=autoland&lineNumber=7635
Flags: needinfo?(gandalf)
(Assignee)

Comment 21

5 months ago
Thanks!

Fixed the former with a switch to `requestAnimationFrame`.
Fixed the latter by switching to manual loading of `appinfo` in L10nRegistry because extension tests rely on overriding `Services.appinfo`.
Flags: needinfo?(gandalf)
Blocks: 1500489

Comment 22

5 months ago
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4f1fd54c853f
Wrap privileged promises in a promise for the document we`re returning to. r=smaug
https://hg.mozilla.org/integration/autoland/rev/755d616d0cb9
Register locale sources in all processes. r=mossop
https://hg.mozilla.org/integration/autoland/rev/08b16a40d372
Add tests for localisation of unprivileged content. r=flod,mossop

Comment 23

5 months ago
Pushed by ncsoregi@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fa8604507541
Followup for eslint failure. CLOSED TREE
You need to log in before you can comment on or make changes to this bug.