1406358 - [Mac 10.13]: Reader Mode TTS not working

Status: VERIFIED FIXED

(Core :: Web Speech, defect)

Description

[Winston]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/61.0.3163.100 Safari/537.36

Steps to reproduce:

Make sure the Mac is running macOS 10.13 (I am not sure about older version but I am using macOS 10.13) and Firefox 56 installed.

1. Run Firefox without any addon (or Safe mode)
2. Go to http://mentalfloss.com/article/504666/bit-bit-inside-rise-retro-gaming
3. After the web page finished loading, enter Reader View
4. After the Reader View finished loading, press the read button of the left and then press the play button


Actual results:

On my Mac it only reads the website domain 'mentalfloss.com' then TTS stops.


Expected results:

It should be able to read the whole article

Comment 1

[Marcia Knous [:marcia]]

I can confirm this doesn't work on 56, 57 beta or 58 on my 10.13 machine. I can confirm that it works on my 10.12 machine on 57 beta. So appears to be a 10.13 only issue.

Comment 2

[Marcia Knous [:marcia]]

Adding tracking flags.

Comment 3

[:Gijs (out for now; he/him)]

Eitan, any idea what's going on here?

Comment 4

[:Gijs (out for now; he/him)]

André, seems like you know about Web Speech, maybe you can help?

Comment 5

[Julien Cristau [:jcristau]]

Doesn't sound like a regression if this is new in 10.13.

Comment 6

[André Natal]

I am more familiar with the recognition part. About TTS I think Eitan is the more appropriated contact.

Comment 8

[Makoto Kato [:m_kato]]

We shouldn't call [delegate release ] after setDelegate.

Comment 9

[Eitan Isaacson [:eeejay]]

Makoto, can you take this bug? Looks like you know the fix.

Comment 10

[Makoto Kato [:m_kato]]

OK, patch is coming.

Comment 11

[Makoto Kato [:m_kato]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc572a711ad0680c5df95b8ef64db9a66d697b93

Comment 12

[Makoto Kato [:m_kato]]

Attachment: Bug 1406358 - Keep SpeechDelegate object until speaking is finished.

The delegate property of NSSpeechSynthesizer doesn't seem to add refcount from 10.13, so we have to keep SpeechDelegate object until speech is finished.

Review commit: https://reviewboard.mozilla.org/r/188664/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/188664/

Comment 13

[Eitan Isaacson [:eeejay]]

Comment on attachment 8917732 [details]
Bug 1406358 - Keep SpeechDelegate object until speaking is finished.

https://reviewboard.mozilla.org/r/188664/#review194150

The ownership here is a bit confusing.. from what I understand:
1. nsSpeechTask holds a strong reference to SpeechTaskCallback (its actually a cyclical reference)
2. SpeechTaskCallback holds a strong reference to the SpeechDelegate (it creates it, and then releases its single reference in the destructor)
3. The SpeechDelegate holds a weak reference to the SpeechTaskCallBack.

Did I get that right? Maybe a comment about (2) when you create SpeechDelegate because the mix of c++/objc reference counting is confusing, and it does not look like an owning reference.

Comment 14

[Makoto Kato [:m_kato]]

(In reply to Eitan Isaacson [:eeejay] from comment #13)
> Comment on attachment 8917732 [details]
> Bug 1406358 - Keep SpeechDelegate object until speaking is finished.
> 
> https://reviewboard.mozilla.org/r/188664/#review194150
> 
> The ownership here is a bit confusing.. from what I understand:
> 1. nsSpeechTask holds a strong reference to SpeechTaskCallback (its actually
> a cyclical reference)
> 2. SpeechTaskCallback holds a strong reference to the SpeechDelegate (it
> creates it, and then releases its single reference in the destructor)
> 3. The SpeechDelegate holds a weak reference to the SpeechTaskCallBack.
> 
> Did I get that right? Maybe a comment about (2) when you create
> SpeechDelegate because the mix of c++/objc reference counting is confusing,
> and it does not look like an owning reference.

10.12 is NSSpeechSynthesizer has strong reference of SpeechDelegate. So when NSSpeechSynthesizer is destroyed in destructor, SpeechDelegate is released and destroyed.  So new code will be same. (destructor destroys both NSSpeechSynthesizer and SpeechDelegate)

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dd361e3ebf4d
Keep SpeechDelegate object until speaking is finished. r=eeejay

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 17

[Julien Cristau [:jcristau]]

Do we need to fix this in 57?  If so please request uplift when you get a chance.

Comment 18

[Makoto Kato [:m_kato]]

Comment on attachment 8917732 [details]
Bug 1406358 - Keep SpeechDelegate object until speaking is finished.

Approval Request Comment
[Feature/Bug causing the regression]:
Upgrade to macOS 10.13

[User impact if declined]:
Web Speech API only speaks first sentence only.

[Is this code covered by automated tests?]:
No.

[Has the fix been verified in Nightly?]:
Yes.

[Needs manual test from QE? If yes, steps to reproduce]: 
Yes, See https://bugzilla.mozilla.org/show_bug.cgi?id=1406358#c0

[List of other uplifts needed for the feature/fix]:
N/A

[Is the change risky?]:
Middle.

[Why is the change risky/not risky?]:
I change reference counting for objective-c object.  It might be memory leak.

[String changes made/needed]:
No.

Comment 19

[Winston]

I tried it on Firefox Nightly and it is working. But the CPU consumption is around 50% on my MBP (the windows version is running at 5%).

Comment 20

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hello guys, the previous comment about the CPU usage on MacBook Pro as compared to the windows OS is worrisome. I'd like to see if this is an easily reproducible issue and related to the patch in this bug or unrelated before making a decision on the beta uplift. Any help is appreciated. Thanks!

Comment 21

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Jim, getting this on your radar too. Thanks!

Comment 22

[Makoto Kato [:m_kato]]

(In reply to Winston from comment #19)
> I tried it on Firefox Nightly and it is working. But the CPU consumption is
> around 50% on my MBP (the windows version is running at 5%).

What is macOS version? 10.13?  and is reproduce step comment #0?

Comment 23

[Makoto Kato [:m_kato]]

Before landing this, TTS stops at 1st sentence.  So CPU consumption spends 1st sentence only.

Comment 24

[Winston]

Makoto, I ran the Firefox Nightly on macOS 10.13.

Yes the test case still valid. The only additional thing is opening the activity monitor (task manager).

You open the article in Reader Mode, open the activity monitor, wait until the CPU load is idle and then press the play button. You will find that the loading will jump to 50%.

Comment 25

[Makoto Kato [:m_kato]]

https://perfht.ml/2zPSwG1

Not TTS issue.  Speech doesn't show on profiler.  But most is painting on content (Narreta will update content for maker everytime).   So comment #19 doesn't depends on this bug.

Comment 26

[Stuart Philp :sphilp]

Just tried this now on 58.0a1 (2017-10-29), OSX 10.13. ~15-35% CPU usage (I'm on a 2017 mbp, may be faster generally and use less CPU. You also want to be careful as a few browser components have a delayed start for the first 60 seconds or so after browser start up, so CPU usage can seemingly spike randomly at first (that's not what's going on here, but just to note, wait at least 60 seconds before measuring).

I ran this as well on 52esr for comparison and get 15-25% CPU usage there, even though the functionality is broken (same as this original bug, it narrates the first line only, then continues to look as if it is narrating, but the actual voice output stops). 

imo this is overall an improvement. Seeing as this functionality has been broken since at least 52, and this patch now has it working, that's worth while itself. If there is a CPU regression, it seems to be < 10% on my machine (hard to say without more in depth profiling exactly). Whether it is acceptable and worth taking an uplift for, that might be a better call for product, but imo it's fine to take. QA should add a regression case for this if we haven't already.

Comment 27

[Winston]

Hi Stuart,

I tried this and that really improve the CPU load.
1. https://developer.microsoft.com/en-us/microsoft-edge/testdrive/demos/speechsynthesis/
2. Copy the text inside the textbox
3. Press the play button

The CPU load will goes from 50% to 10% (it differs from laptop to laptop, but the improvement is significant).

So it makes me think that the problem is inside Reader Mode. But have no idea which part is it.

Comment 28

[Stuart Philp :sphilp]

That's awesome, thanks Winston. What might be appropriate here is we file a follow up to investigate what in reader view is eating up the CPU (since this bug is about functionality), what do you think Ritu?

Comment 29

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

I agree that it should be a follow up bug. Thanks for your due diligence Stuart. I'll take this patch in 57.

Comment 30

[Makoto Kato [:m_kato]]

(In reply to Stuart Philp :sphilp from comment #28)
> That's awesome, thanks Winston. What might be appropriate here is we file a
> follow up to investigate what in reader view is eating up the CPU (since
> this bug is about functionality), what do you think Ritu?

Winston filed as Bug 1406360.

Comment 31

[Winston]

Makoto, thanks for finding the root cause of the high CPU load.

I think it is better to have your findings (perf.html) filed in bug 1406360.

Thanks

Comment 32

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8917732 [details]
Bug 1406358 - Keep SpeechDelegate object until speaking is finished.

Recent regression, Beta57+

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/24b1d36cd710

Comment 34

[Camelia Badau [:cbadau], Desktop Test Engineering]

I've reproduced the initial issue (steps from Description) on macOS 10.13.1 using Firefox 56.0. 

I've tested on macOS 10.13.1 using Firefox 57 Beta 14 (buildID: 20171102181127) and latest Nightly 58.0a1 (2017-11-03) and Reader Mode TTS is correctly working, all the text is read.