Closed Bug 1129106 Opened 5 years ago Closed 5 years ago

Load about:reader in the child process

Categories

(Firefox :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 38
Iteration:
38.3 - 23 Feb
Tracking Status
firefox38 --- fixed

People

(Reporter: Margaret, Assigned: abdelrahman, Mentored)

References

Details

(Whiteboard: [good first bug])

Attachments

(1 file)

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.
Flags: firefox-backlog+
Attachment #8561012 - Flags: review?(margaret.leibovic)
Assignee: nobody → a.ahmed1026
Status: NEW → ASSIGNED
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?
Flags: needinfo?(a.ahmed1026)
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.
Attachment #8561012 - Flags: review?(margaret.leibovic) → feedback+
(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.
Flags: needinfo?(a.ahmed1026)
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?
Flags: needinfo?(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.
Flags: needinfo?(mconley)
(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?
Flags: needinfo?(a.ahmed1026)
(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.
Flags: needinfo?(a.ahmed1026)
Depends on: 1132925
(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.
(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?
(In reply to Abdelrhman Ahmed from comment #10)
> Sure :). Is there any documentation for something like that?

Don't bother yourself. I got it.
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?
(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 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/
Attachment #8561012 - Flags: feedback+ → review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/ab63972551cb
Keywords: checkin-needed
Whiteboard: [good first bug] → [good first bug][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/ab63972551cb
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][fixed-in-fx-team] → [good first bug]
Target Milestone: --- → Firefox 38
Iteration: --- → 38.3 - 23 Feb
Flags: qe-verify?
You need to log in before you can comment on or make changes to this bug.