1129106 - Load about:reader in the child process

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[:Margaret Leibovic]

After bug 1083281 lands, we should move about:reader to load in the child process. I designed it to be able to run in the child process, so this *should* work.

You'll need to change the file here:
http://mxr.mozilla.org/mozilla-central/source/browser/components/about/AboutRedirector.cpp#118

See bug 1083281 for the flag we need to use.

Comment 1

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 1 - Load about:reader in the child process

Comment 2

[:Margaret Leibovic]

While this patch does look correct, it looks like about:reader doesn't actually work as expected in the child process :/

Abdelrhman, would you be interested in trying to debug this?

Comment 3

[:Margaret Leibovic]

Comment on attachment 8561012 [details] [diff] [review]
rev 1 - Load about:reader in the child process

feedback+ for now, since we can't land this until about:reader actually works in the child process.

Comment 4

[Abdelrahman Ahmed [:abdelrahman]]

(In reply to :Margaret Leibovic from comment #2)
> Abdelrhman, would you be interested in trying to debug this?

Sure :)

> While this patch does look correct, it looks like about:reader doesn't
> actually work as expected in the child process :/

This issue comes from using |Services.prefs.set| methods in
toolkit/components/reader/AboutReader.jsm like
> Services.prefs.setIntPref("reader.font_size", this._fontSize);

and according to modules/libpref/nsPrefBranch.cpp set methods
(SetIntPref, SetCharPref, SetBoolPref) needs to be called from
main process not child.
> ENSURE_MAIN_PROCESS("Cannot SetBoolPref from content process:", aPrefName);

So I think we can allow these methods to be called from child process
by removing this check like get methods (GetBoolPref, GetCharPref, GetIntPref)
but I'm not sure if this may cause security risks.

Comment 5

[:Margaret Leibovic]

Thanks for looking into this!

This is sad news for me, because we rely pretty heavily on Services.prefs to control the about:reader UI. We could add some sort of helper to pass messages to the parent to set prefs, I actually wonder if something like this already exists for e10s code.

mconley, have you ever run into an issue like this trying to use Services.prefs in the child process? Is there a best practice to follow here?

Comment 6

[Mike Conley (:mconley) (:⚙️)]

(In reply to :Margaret Leibovic from comment #5)
> Thanks for looking into this!
> 
> This is sad news for me, because we rely pretty heavily on Services.prefs to
> control the about:reader UI. We could add some sort of helper to pass
> messages to the parent to set prefs, I actually wonder if something like
> this already exists for e10s code.
> 
> mconley, have you ever run into an issue like this trying to use
> Services.prefs in the child process? Is there a best practice to follow here?

There shouldn't be an issue _reading_ prefs from the content process. Writing, however, is a different story. Writing to prefs is not going to work in the content process - writing must occur in the parent process.

When prefs are updated in the parent, all of the children are notified, and their internal mirrors of all prefs are updated with any changes.

Comment 7

[:Margaret Leibovic]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #6)
> (In reply to :Margaret Leibovic from comment #5)
> > Thanks for looking into this!
> > 
> > This is sad news for me, because we rely pretty heavily on Services.prefs to
> > control the about:reader UI. We could add some sort of helper to pass
> > messages to the parent to set prefs, I actually wonder if something like
> > this already exists for e10s code.
> > 
> > mconley, have you ever run into an issue like this trying to use
> > Services.prefs in the child process? Is there a best practice to follow here?
> 
> There shouldn't be an issue _reading_ prefs from the content process.
> Writing, however, is a different story. Writing to prefs is not going to
> work in the content process - writing must occur in the parent process.
> 
> When prefs are updated in the parent, all of the children are notified, and
> their internal mirrors of all prefs are updated with any changes.

Aha, thanks, this makes this problem a bit simpler. I would be ok with sending messages to write those prefs, because that shouldn't happen often. Also, setting "reader.has_used_toolbar" should really be moved into the parent process on Android, since that's only used for Android, not desktop (I'll file a separate bug about this).

Abdelrhman, can you experiment with commenting out those setCharPref/setIntPref/setBoolPref calls in AboutReader.jsm to see if that makes loading about:reader in the child process work?

Comment 8

[Abdelrahman Ahmed [:abdelrahman]]

(In reply to :Margaret Leibovic from comment #7)
> Abdelrhman, can you experiment with commenting out those
> setCharPref/setIntPref/setBoolPref calls in AboutReader.jsm to see if that
> makes loading about:reader in the child process work?

It works fine.

Comment 9

[:Margaret Leibovic]

(In reply to Abdelrhman Ahmed from comment #8)
> (In reply to :Margaret Leibovic from comment #7)
> > Abdelrhman, can you experiment with commenting out those
> > setCharPref/setIntPref/setBoolPref calls in AboutReader.jsm to see if that
> > makes loading about:reader in the child process work?
> 
> It works fine.

Excellent! I filed bug 1132925 to fix this for real. Are you interested in taking that bug? It would be a bit harder than this one, but I can help you.

Comment 10

[Abdelrahman Ahmed [:abdelrahman]]

(In reply to :Margaret Leibovic from comment #9)
> Excellent! I filed bug 1132925 to fix this for real. Are you interested in
> taking that bug? It would be a bit harder than this one, but I can help you.

Sure :). Is there any documentation for something like that?

Comment 11

[Abdelrahman Ahmed [:abdelrahman]]

(In reply to Abdelrhman Ahmed from comment #10)
> Sure :). Is there any documentation for something like that?

Don't bother yourself. I got it.

Comment 12

[:Margaret Leibovic]

I just landed the patch for bug 1132925, so hopefully with that things will all work well here. Can you test things out to verify that?

Comment 13

[Abdelrahman Ahmed [:abdelrahman]]

(In reply to :Margaret Leibovic from comment #12)
> I just landed the patch for bug 1132925, so hopefully with that things will
> all work well here. Can you test things out to verify that?

Prefs are set successfully in child process and
reader works fine but not tested on Android.

Comment 14

[:Margaret Leibovic]

Comment on attachment 8561012 [details] [diff] [review]
rev 1 - Load about:reader in the child process

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

Nice! I just tested this out with the patch for bug 1132925 applied and it works well.

I also had my patch for bug 1130206 applied, and together these patches solve the re-downloading-content problem with e10s enabled as well \o/

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/ab63972551cb

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ab63972551cb