1302099 - GMPInstallManager logs an error during linux-asan mochitest-bc

Status: RESOLVED INACTIVE

(Toolkit :: Add-ons Manager, defect, P5)

Description

[Chris H-C :chutten]

JavaScript Error: "1473454676065	Toolkit.GMP	ERROR	GMPInstallManager.simpleCheckAndInstall Could not check for addons: Error: got node name: html, expected: updates (resource://gre/modules/addons/ProductAddonChecker.jsm:153:11) JS Stack trace: [e-mail]:153:11 < Async*[e-mail]:320:12 < [e-mail]:107:5 < GMPInstallManager.prototype.simpleCheckAndInstall<@GMPInstallManager.jsm:204:29 < gBrowserInit._delayedStartup/<@browser.js:1315:7 < setTimeout handler*[e-mail]:1311:5 < EventListener.handleEvent*[e-mail]:1001:5 < [e-mail]:1:1" {file: "resource://gre/modules/Log.jsm" line: 753}

Seems like there's something a little off in how GMPInstallManager handles being run in a test.

...or is it a real error? It doesn't seem to be fatal.

An example, from tryrun [1], a log [2]

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b602e2d0918d&selectedJob=27233009
[2]: https://public-artifacts.taskcluster.net/YwNrg39fSzOmF85phx3pnA/0/public/logs/live_backing.log

Comment 1

[:Gijs (he/him)]

As noted on #developers, I think this is an artifact of our mochitest "redirect all our remote requests to localhost" stuff, where GMPInstallManager is now being served an HTML page by the mochitest http server (probably a 404?) and is not happy about this.

Off the top of my head, we could:
- make it get a real thing (that says "no updates"?) off the mochitest http server so it's happy.
- not enable gmp error-level logging in mochitests
- make it check for this case and not raise an Error-level log message.

Kirk, seems like you've poked at this code a bit in recent history. What would be the best way forward here?

Comment 2

[Robin Steuber (she/her) [:bytesized]]

I agree with your possible solutions. I don't know how difficult it would be to have the mochitest http server serve an updates file, but it seems like preventing the error would be better than suppressing it. I believe that the updates file would just need to contain "<updates><addons></addons></updates>" to prevent the errors. The one part that may be tricky though (depending how mochitests serve http), is that the URL for the update file is a template URL:

> https://aus5.mozilla.org/update/3/GMP/%VERSION%/%BUILD_ID%/%BUILD_TARGET%/%LOCALE%/%CHANNEL%/%OS_VERSION%/%DISTRIBUTION%/%DISTRIBUTION_VERSION%/update.xml

May be converted to:

> https://aus5.mozilla.org/update/3/GMP/47.0a1/20160222160130/WINNT_x86_64-msvc-x64/en-US/default/Windows_NT%2010.0.0.0%20(x64)/default/default/update.xml

Comment 3

[:Gijs (he/him)]

(In reply to Kirk Steuber [:bytesized] from comment #2)
> I agree with your possible solutions. I don't know how difficult it would be
> to have the mochitest http server serve an updates file, but it seems like
> preventing the error would be better than suppressing it. I believe that the
> updates file would just need to contain
> "<updates><addons></addons></updates>" to prevent the errors. The one part
> that may be tricky though (depending how mochitests serve http), is that the
> URL for the update file is a template URL:
> 
> > https://aus5.mozilla.org/update/3/GMP/%VERSION%/%BUILD_ID%/%BUILD_TARGET%/%LOCALE%/%CHANNEL%/%OS_VERSION%/%DISTRIBUTION%/%DISTRIBUTION_VERSION%/update.xml
> 
> May be converted to:
> 
> > https://aus5.mozilla.org/update/3/GMP/47.0a1/20160222160130/WINNT_x86_64-msvc-x64/en-US/default/Windows_NT%2010.0.0.0%20(x64)/default/default/update.xml

Well, the pref can simply not contain those variables. It seems we currently set it to:

http://searchfox.org/mozilla-central/source/testing/profiles/prefs_general.js#49

which is just a fixed URL on localhost.

We'd just need to teach it to actually serve a thing at that URL. I'm not sure how to do that off-hand. Dave, do you know how that stuff works?

Comment 4

[Robert Helmer [:rhelmer]]

(In reply to Kirk Steuber [:bytesized] from comment #2)
> I agree with your possible solutions. I don't know how difficult it would be
> to have the mochitest http server serve an updates file, but it seems like
> preventing the error would be better than suppressing it. I believe that the
> updates file would just need to contain
> "<updates><addons></addons></updates>" to prevent the errors. The one part
> that may be tricky though (depending how mochitests serve http), is that the
> URL for the update file is a template URL:
> 
> > https://aus5.mozilla.org/update/3/GMP/%VERSION%/%BUILD_ID%/%BUILD_TARGET%/%LOCALE%/%CHANNEL%/%OS_VERSION%/%DISTRIBUTION%/%DISTRIBUTION_VERSION%/update.xml
> 
> May be converted to:
> 
> > https://aus5.mozilla.org/update/3/GMP/47.0a1/20160222160130/WINNT_x86_64-msvc-x64/en-US/default/Windows_NT%2010.0.0.0%20(x64)/default/default/update.xml

This URL is set by the pref `media.gmp-manager.url`, modifying this to point to an URL that is overridden to serve an empty XML update would be one approach. 

It might be better to just log this as a single-line warning though ("Invalid response checking for Gecko Media Plugin updates"), since there isn't really anything user-actionable if this is failing so logging a big ugly exception seems unwarranted.

The exception.message could be logged as a debug message perhaps.

Comment 5

[Robert Helmer [:rhelmer]]

(In reply to :Gijs Kruitbosch from comment #3)
> (In reply to Kirk Steuber [:bytesized] from comment #2)
> > I agree with your possible solutions. I don't know how difficult it would be
> > to have the mochitest http server serve an updates file, but it seems like
> > preventing the error would be better than suppressing it. I believe that the
> > updates file would just need to contain
> > "<updates><addons></addons></updates>" to prevent the errors. The one part
> > that may be tricky though (depending how mochitests serve http), is that the
> > URL for the update file is a template URL:
> > 
> > > https://aus5.mozilla.org/update/3/GMP/%VERSION%/%BUILD_ID%/%BUILD_TARGET%/%LOCALE%/%CHANNEL%/%OS_VERSION%/%DISTRIBUTION%/%DISTRIBUTION_VERSION%/update.xml
> > 
> > May be converted to:
> > 
> > > https://aus5.mozilla.org/update/3/GMP/47.0a1/20160222160130/WINNT_x86_64-msvc-x64/en-US/default/Windows_NT%2010.0.0.0%20(x64)/default/default/update.xml
> 
> Well, the pref can simply not contain those variables. It seems we currently
> set it to:
> 
> http://searchfox.org/mozilla-central/source/testing/profiles/prefs_general.
> js#49
> 
> which is just a fixed URL on localhost.
> 
> We'd just need to teach it to actually serve a thing at that URL. I'm not
> sure how to do that off-hand. Dave, do you know how that stuff works?

You could either have a real XML file on disk, or just generate it like:

  testserver.registerPathHandler("/data/update.xml", (request, response) => {
    response.write("<updates><addons/></updates>");
  });

Assuming the pref was pointing at localhost/data/update.xml

Comment 6

[BMO Automation]

Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.