mozilla-extensions / firefox-voice

Firefox Voice is an experiment in a voice-controlled web user agent
Mozilla Public License 2.0
285 stars 122 forks source link

"Play Green Day" displays error in doorhanger when first made #1189

Open alexandra-martin opened 4 years ago

alexandra-martin commented 4 years ago

Prerequisites:

Mic permissions are enabled. "Allow" or "Don't Allow" button from “Allow Firefox Voice to Collect Voice Transcriptions” pop-up needs to be clicked. An account is logged in on Spotify and Autoplay is enabled.

STR:

  1. Use the shortcut or click on the mic icon from the browser toolbar.
  2. Say or write the "Play Green Day" command.

Expected result:

Spotify opens in a new tab and starts playing the desired music.

Actual result:

Spotify opens in a new tab, but an error is displayed in the doorhanger and music is not playing.

Notes:

Reproduced on Mac 10.14 and Win10x64 with Firefox Nightly 75.0a1 (2020-03-09). On Mac the error displayed is "Internal error: Error: Timeout waiting for #searchPage button[aria-label='Play'], .tracklist .tracklist-play-pause". On Windows the error displayed was either the one displayed on Mac or "Internal error: Error: No matching message handler". In the Browser Console the "NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIInterfaceRequestor.getInterface]" error is displayed. If "Play Green Day" command is made a second time while the Spotify tab is active, the command will work as intended.

4

19

SaumyaSinghal commented 4 years ago

@ianb I was able to reproduce it on Linux. I'd like to work on it. Can you please suggest the files in which changes need to be made?

ianb commented 4 years ago

@SaumyaSinghal Probably in either extension/services/spotify/player.js or spotify.js – it might simply be that the timeout is too short, or maybe spotify.js needs to wait for the page to load (look for browserUtil.waitForDocumentComplete() for some examples of how to wait). Or if Spotify "loads" but is still doing a bunch of work, maybe there's a way to detect that it hasn't rendered yet and add to the timeout.

SaumyaSinghal commented 4 years ago

Okay Thank You @ianb

SaumyaSinghal commented 4 years ago

@ianb, I don't think that this error is limited to "play SONG". I tried many commands and if the command is "search SONG on spotify" then also an error "Internal error: Error: No matching message handler" displays. Again, if the search command is made a second time then it works as intended. I also faced another error, if the first command (just after npm start when nightly opens) is "Play SONG" then an error "Internal error: Error: Missing host permission for the tab" is displayed. I tried to change the timeout in player.js and spotify.js but the error was still shown. Can you please guide me with this? I think I am missing out on something?

ianb commented 4 years ago

@SaumyaSinghal If it's an error about a message handler, then that means that the code is sending a message to the tab, but there are no scripts attached to the tab to listen. content.lazyInject adds scripts to the page, but if the page reloads or navigates somewhere new (on its own) then those scripts will be unloaded. Attempts to send messages then won't work. So that's one possibility: there's a page change and the scripts need to be re-added. Catching that error and re-adding the scripts would be one possible fix.

"Missing host permission" is a bit strange. That's generally if you try to add a script or do something with a privileged page. Usually an about:* page. It's possible that it's trying to add something to about:blank? Usually when a tab is created it starts on about:blank – too short to see, but the extension might catch it at that moment. The browserUtil.loadUrl() function specifically tries to work around that, but I notice the code calls context.createTab(), which just calls browserUtil.createTab() and it doesn't have that same logic. So maybe the solution is to ensure that if createTab(options) specifies an options.url, that we wait for the URL to really be loaded before returning.

Now that I write it down, I feel more like fixing createTab could be the right answer here, and might fix other things in the future too.