Closed Bug 1124011 Opened 9 years ago Closed 9 years ago

Enable the reader mode toolbar button on Nightly

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 39
Iteration:
39.1 - 9 Mar
Tracking Status
firefox38 --- fixed
firefox39 --- verified

People

(Reporter: Margaret, Assigned: Margaret)

References

Details

Attachments

(1 file)

Filing this now, since we'll want to do this eventually.

I think we can go ahead with this once we fix bug 1117258 and bug 1120735.

In the meantime, starting tomorrow you should be able to flip "reader.parse-on-load.enabled" to true on Nightly to try things out.
Flags: firefox-backlog+
Setting this dependent on the two buttons that don't do anything right now in the toolbar (bug 1133485 and bug 1133489).  Unless we want to temporarily hide those buttons?  With those fixed I think we should turn this on in Nightly.
Depends on: 1133485, 1133489
I think it will be a while until those buttons start working, since they depend on actually having a reading list. Blair, do you agree?

I can write a patch to disable those buttons and enable the toolbar button in Nightly.
Flags: needinfo?(bmcbride)
(In reply to :Margaret Leibovic from comment #2)
> I think it will be a while until those buttons start working, since they
> depend on actually having a reading list. Blair, do you agree?

Yep. We'll be landing pieces of the ReadingList as soon as we can, but it's just UI with no brains behind it. It won't be in a state where it can actually be enabled for quite some time.

The entire ReadingList feature is gated behind the browser.readinglist.enabled pref - it'd probably make sense to make those two buttons depend on that pref too.
Flags: needinfo?(bmcbride)
(In reply to :Margaret Leibovic from comment #2)
> I can write a patch to disable those buttons and enable the toolbar button
> in Nightly.

Sounds good, it would be great to get some testing started on reader mode.
Dropping bug 1133485 and bug 1133489 from dependencies, looking for a patch from margaret to hide those buttons behind the browser.readinglist.enabled pref instead.

Margaret: are you going to enable this on Nightly in the same patch?

I'm reaching out to Clint today to figure a testing plan for Reader View.  It feels like this feature might need a more targeted approach to testing; ensuring we work well with a range of popular sites. I'll cc the both of you on that mail.
No longer depends on: 1133485, 1133489
Flags: needinfo?(margaret.leibovic)
Yes, I'll work on writing a patch to hide the reading list buttons behind the reading list pref, and I can enable reader view in nightly in the same patch.

Stay tuned, hopefully I'll have something ready for review by this afternoon.
Flags: needinfo?(margaret.leibovic)
I built this on top of my patch for bug 1131303, since that added JS logic for setting up the "open/close reading list" button.

(Using an old-school patch, since mozreview still doesn't handle dependent reviews well)
Attachment #8566684 - Flags: review?(bmcbride)
(In reply to :Margaret Leibovic from comment #7)
> (Using an old-school patch, since mozreview still doesn't handle dependent
> reviews well)

Even with bug 1097213 landed?
Comment on attachment 8566684 [details] [diff] [review]
Hide reader view buttons related to reading list if reading list is disabled, and enable reader view by default on Nightly

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

r- until clarification of following:

::: mobile/android/app/mobile.js
@@ +869,5 @@
>  // Whether to use a vertical or horizontal toolbar.
>  pref("reader.toolbar.vertical", false);
> +
> +// Whether or not to display buttons related to reading list in reader view.
> +pref("browser.readinglist.enabled", true);

Er, this doesn't seem to make sense.
Attachment #8566684 - Flags: review?(bmcbride) → review-
(In reply to Blair McBride [:Unfocused] (I don't read bugmail - needinfo? me!) from comment #8)
> (In reply to :Margaret Leibovic from comment #7)
> > (Using an old-school patch, since mozreview still doesn't handle dependent
> > reviews well)
> 
> Even with bug 1097213 landed?

Well I uploaded this before that bug landed :)

(In reply to Blair McBride [:Unfocused] (I don't read bugmail - needinfo? me!) from comment #9)
> Comment on attachment 8566684 [details] [diff] [review]
> Hide reader view buttons related to reading list if reading list is
> disabled, and enable reader view by default on Nightly
> 
> Review of attachment 8566684 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r- until clarification of following:
> 
> ::: mobile/android/app/mobile.js
> @@ +869,5 @@
> >  // Whether to use a vertical or horizontal toolbar.
> >  pref("reader.toolbar.vertical", false);
> > +
> > +// Whether or not to display buttons related to reading list in reader view.
> > +pref("browser.readinglist.enabled", true);
> 
> Er, this doesn't seem to make sense.

For Android the reading list is enabled no matter what, so on Android this pref literally just only controls those buttons, where this pref is read in shared code.

I suppose I could add the pref to firefox.js (or maybe you landed a patch that did that now?) and update the logic to just show those buttons if the pref can't be found.
So, given my last comment, what do you think I should do? :)
Flags: needinfo?(bmcbride)
Comment on attachment 8566684 [details] [diff] [review]
Hide reader view buttons related to reading list if reading list is disabled, and enable reader view by default on Nightly

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

Oh, are those buttons not desktop-only? I'd been assuming they were, as that's what the bug that added them seemed to imply (and I didn't notice any obvious way they were hooked up on mobile.

So if they're not desktop only, r+

If they are desktop-only, them I'm still confused by that line.
Attachment #8566684 - Flags: review- → review+
Flags: needinfo?(bmcbride)
(In reply to Blair McBride [:Unfocused] (I don't read bugmail - needinfo? me!) from comment #12)
> Comment on attachment 8566684 [details] [diff] [review]
> Hide reader view buttons related to reading list if reading list is
> disabled, and enable reader view by default on Nightly
> 
> Review of attachment 8566684 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Oh, are those buttons not desktop-only? I'd been assuming they were, as
> that's what the bug that added them seemed to imply (and I didn't notice any
> obvious way they were hooked up on mobile.
> 
> So if they're not desktop only, r+
> 
> If they are desktop-only, them I'm still confused by that line.

Yes, they are totally shared. They existed on mobile first, and then I just added CSS to style them for desktop.

Be aware that everything in AboutReader.jsm and aboutReader.html are shared, so if you ever make a change in there, you need to test it on both desktop and Android.
https://hg.mozilla.org/mozilla-central/rev/66cf24f275b7
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
Iteration: --- → 38.3 - 23 Feb
Flags: qe-verify?
This shouldn't have been merged to m-c per comment 15. At this point, m-c is tagged for uplift to Aurora, so I'm going to manually land the backout there post-merge.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 38 → ---
Iteration: 38.3 - 23 Feb → 39.1 - 9 Mar
(In reply to Phil Ringnalda (:philor) from comment #15)
> Backed out in https://hg.mozilla.org/integration/fx-team/rev/28ff18b517ee
> for frequent ASan crashes in test_memoryReporters.xul like
> https://treeherder.mozilla.org/logviewer.html#?job_id=2064715&repo=fx-team

I made a try push to confirm that this patch really did cause this problem, and it looks like it does:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8cea2e4ae9bc

njn, could you help me understand what test_memoryReporters.xul does, and perhaps offer any insights into why enabling reader mode might be causing it to crash frequently?

The main thing I can think of is that with "reader.parse-on-load.enabled" set to true, we call ReaderMode.parseDocument on pageshow, which creates a worker thread to parse the document, and likely uses a decent amount of memory.

I wonder if we try to parse test pages that are loaded in the browser... but if that caused problems I imagine that would cause more test failures than just this memory test.
Flags: needinfo?(n.nethercote)
Also worth noting: bug is an existing intermittent issue on file that may be related to this crash. Maybe I just came across a way to reliably reproduce that intermittent issue?
(In reply to :Margaret Leibovic from comment #20)
> Also worth noting: bug is an existing intermittent issue on file that may be
> related to this crash. Maybe I just came across a way to reliably reproduce
> that intermittent issue?

Oops, that was supposed to say bug 1118771.
test_memoryReporters.xul basically just runs all the memory reporters to make sure that none of them crash. Given that your patch only modifies JS and HTML, it looks like you've tickled a latent bug of some kind. I initially thought that one of the memory reporters must be buggy, but the stack traces make it look like the problem is in the code that invokes the memory reporters... which is really surprising. 

It could be related to the new worker, not because of the amount of memory it uses, but because the worker memory reporter code is a PITA that has had bugs in the past, albeit none that have looked like this.

The stack traces don't match those in bug 1118771, so I don't think that's relevant.

That's all I have time for now, but I'll take a look again tomorrow. I'm sorry that you're being blocked by something that's clearly not your fault :(
Flags: needinfo?(n.nethercote)
I've determined that it's a problem with the worker memory reporter.

For some reporters the memory reporter manager holds them with a strong pointer, and for others it uses a weak pointer. For worker reporters it's a weak pointer. If I convert it to a strong pointer (which will cause the reporter to leak) then the crash stops happening. Likewise if I simply disable the worker memory reporters.

So it very much looks like a worker memory reporter is destroyed but the memory reporter manager still holds a pointer to it and then derefs that pointer. I can't yet see how this is possible, but the worker reporter code is complex so there must be a problem there.

Unfortunately I can't reproduce this with a local ASan build so I'm having to do try runs to investigate. It's Friday afternoon here now; I'll take another look on Monday.
I think I'm close to working this out. It looks like a problem with the temporary blocking of reporter registration/unregistration that gets done just during tests. Which is good news, because that would mean it's a test bug and not something we'd see in the wild. More tomorrow.
Depends on: 1138770
Bug 1138770's patch just landed on m-i. You should be clear to re-land, Margaret. Sorry again for the inconvenience.
(In reply to Nicholas Nethercote [:njn] from comment #25)
> Bug 1138770's patch just landed on m-i. You should be clear to re-land,
> Margaret. Sorry again for the inconvenience.

Thanks so much for investigating! I appreciate you making this a priority.

Re-landed:
https://hg.mozilla.org/integration/fx-team/rev/5cd9065e6cad
https://hg.mozilla.org/mozilla-central/rev/5cd9065e6cad
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
Flags: qe-verify? → qe-verify+
QA Contact: andrei.vaida
Depends on: 1139678
Reader View passed Smoke testing on Nightly 39.0a1 (2015-03-04), with known issues already on file. Testing was done on Ubuntu 14.04 (x64), Windows 7 (x64) and Mac OS X 10.9.5. 

'reader.parse-on-load.enabled' is now true by default on m-c.
Status: RESOLVED → VERIFIED
I uplifted this because other patches I needed to uplift depended on it. However, this won't enable the button on Aurora, since it's still behind a Nightly #ifdef.

https://hg.mozilla.org/releases/mozilla-aurora/rev/d828b6d179dd
Depends on: 1139250
According to comment 29 this does not need additional QA on Firefox 38.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: