Closed Bug 1042161 Opened 6 years ago Closed 5 years ago

Handle OpenH264 updates for long-running sessions

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set
Points:
3

Tracking

()

VERIFIED FIXED
mozilla34
Iteration:
34.2
Tracking Status
firefox33 --- verified
firefox34 --- verified

People

(Reporter: gfritzsche, Assigned: gfritzsche)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 2 obsolete files)

We have to consider how to deal with automatic OpenH264 updates for long-running sessions. AFAIR we currently only check & trigger these once on startup.

For long-running sessions we could either go with UPDATE_WHEN_PERIODIC_UPDATE (see below) or utilize update timers directly in browser/ code with GMPInstallManager.

> (Blair McBride [:Unfocused] from bug 1039226, comment #5)
> > Should let through UPDATE_WHEN_PERIODIC_UPDATE too - some people leave their
> > browsers open for a Very Long Time, so the automatic check in browser.js
> > doesn't help much there (this was mentioned somewhere in bug 1009816).
> 
> Forgot to mention - the background update should only go ahead if the
> on-startup check hasn't happened in the past day.
> 
> And you *DO* need to check the state of applyBackgroundUpdates when doing
> background update checks. AddonManager.shouldAutoUpdate() is a useful helper
> function here. AddonManager.jsm normally does this for you, but only if you
> use onUpdateAvailable and implement AddonInstall.
Flags: firefox-backlog+
I'm not sure if this still holds, but based on Bug 1009816 Comment 6 the updates were going to be tightly coupled to Firefox releases.  Which would mean that there was a restart of the browser from the update. 

However if there was a failed update attempt it would have to wait for another restart. That should be fixed but should be rare.   

I'm not against doing this though, I just wanted to mention that since it was the main reason I checked only once at startup.
(In reply to Brian R. Bondy [:bbondy] from comment #1)
> However if there was a failed update attempt it would have to wait for
> another restart. That should be fixed but should be rare.   

Indeed, i don't want us to block on it, but we should fix this later.
Whiteboard: [openh264-uplift]
Re [openh264-uplift]: This bug is "nice-to-have" but not blocking.
I wouldn't block on this, but I think we really want this.  When do we think someone can start working on this?
Flags: needinfo?(georg.fritzsche)
Mainly depends on the effort required here.

Blair, if we just let through UPDATE_WHEN_PERIODIC_UPDATE, what does this imply? How often and under which circumstances will this get triggered?
Flags: needinfo?(georg.fritzsche) → needinfo?(bmcbride)
(In reply to Georg Fritzsche [:gfritzsche] from comment #5)
> Mainly depends on the effort required here.
> 
> Blair, if we just let through UPDATE_WHEN_PERIODIC_UPDATE, what does this
> imply?

Since we're not implementing AddonInstall here, you'll need to check the result of AddonManager.shouldAutoUpdate() if the reason is UPDATE_WHEN_PERIODIC_UPDATE.

Also, since we also have the check just after browser startup, we'd need to do an additional check to ensure it's been at least 1 day since the media.gmp-manager.lastCheck timestamp.

Normally the reason is only used for metrics. If we're counting hits on the update URL (no idea if we are or not), we probably want to include the reason as part of the URL.


> How often and under which circumstances will this get triggered?

It's scheduled via nsUpdateTimerManager.js (the same mechanism the app update uses), which tracks timers between app restarts, and staggers operations. The interval used is one day (ie, minimum time between checks is one day).
Flags: needinfo?(bmcbride)
Thanks, sounds feasible to use this and get it done today then.
Assignee: nobody → georg.fritzsche
Status: NEW → ASSIGNED
Iteration: --- → 34.1
Points: 5 → 3
Summary: Decide on how to handle OpenH264 updates for long-running sessions → Handle OpenH264 updates for long-running sessions
Added to Iteration 34.1
QA Whiteboard: [qa?]
QA Whiteboard: [qa?] → [qa+]
QA Contact: drno
* handles UPDATE_WHEN_PERIODIC_UPDATE
* makes findUpdates() return a Promise<Boolean> to ease testing
* fixes two minor left-overs
Attachment #8466161 - Flags: review?(bmcbride)
Blair, can you advise on how to trigger UPDATE_WHEN_PERIODIC_UPDATE easily for manual QA?
(In reply to Georg Fritzsche [:gfritzsche] from comment #10)
> Blair, can you advise on how to trigger UPDATE_WHEN_PERIODIC_UPDATE easily
> for manual QA?

(1) Open about:addons
(2) Open the DevTools console for that tab
(3) Enter the following: AddonManagerPrivate.backgroundUpdateCheck()
Attachment #8466161 - Flags: review?(bmcbride) → review+
Also xpcshell MOZ_CRASHes due to making external connections during automation:

https://tbpl.mozilla.org/php/getParsedLog.php?id=45166147&tree=Mozilla-Inbound
03:56:06     INFO -  1407149765822	GMPInstallManager._getURL	INFO	Using url: https://aus4.mozilla.org/update/3/GMP/%VERSION%/%BUILD_ID%/%BUILD_TARGET%/%LOCALE%/%CHANNEL%/%OS_VERSION%/%DISTRIBUTION%/%DISTRIBUTION_VERSION%/update.xml
03:56:06     INFO -  1407149765827	GMPInstallManager._getURL	INFO	Using url (with replacement): https://aus4.mozilla.org/update/3/GMP/1/2007010101/XPCShell_noarch-spidermonkey/en-US/default/Windows_NT%206.2.0.0%20(x64)/default/default/update.xml
03:56:06     INFO -  1407149765830	GMPInstallManager.checkForAddons	INFO	sending request to: https://aus4.mozilla.org/update/3/GMP/1/2007010101/XPCShell_noarch-spidermonkey/en-US/default/Windows_NT%206.2.0.0%20(x64)/default/default/update.xml
03:56:06     INFO -  1407149765832	addons.manager	DEBUG	onUpdateFinished for {e2c52c1c-5ee1-cc23-15fa-35945fd58806}
03:56:06     INFO -  1407149765832	addons.manager	DEBUG	onUpdateFinished for {a4254731-ee3a-a1a3-c7e3-22006ca21b3c}
03:56:06     INFO -  1407149765833	addons.manager	DEBUG	onUpdateFinished for {77a62e7b-7c5c-cbe4-bdc6-c8085a32dfdb}
03:56:06     INFO -  1407149765839	addons.update-checker	DEBUG	Requesting http://127.0.0.1/updateBackgroundURL
03:56:06     INFO -  1407149765843	addons.update-checker	DEBUG	Requesting http://127.0.0.1/updateBackgroundURL
03:56:06     INFO -  1407149765847	addons.update-checker	DEBUG	Requesting http://127.0.0.1/updateBackgroundURL
03:56:06     INFO -  Non-local network connections are disabled and a connection attempt to aus4.mozilla.org (63.245.217.219) was made.  You should only access hostnames available via the test networking proxy (if running mochitests) or from a test-specific httpd.js server (if running xpcshell tests). Browser services should be disabled or redirected to a local server.
Attachment #8466161 - Attachment is obsolete: true
Attachment #8467160 - Flags: review+
Attached patch Set dummy URL for GMP updates (obsolete) — Splinter Review
Set dummy URL and use memoized prefs.
Attachment #8467163 - Flags: review?(jmaher)
Comment on attachment 8467163 [details] [diff] [review]
Set dummy URL for GMP updates

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

I am not sure if services.jsm should be included in xpcshell, I really enjoy the simplicity of it- if this is solid on try server (all platforms opt+debug), then I am fine leaving it in.

::: testing/xpcshell/head.js
@@ +1338,5 @@
> +
> +// We need to avoid hitting the network with certain components.
> +if (runningInParent) {
> +  Services.prefs.setCharPref("media.gmp-manager.url", "https://%(server)s/dummy.xml");
> +}

the other pref settings are inside a try/catch block, lets be consistent here.
Attachment #8467163 - Flags: review?(jmaher) → review+
Iteration: 34.1 → 34.2
Some failures with the second patch that i don't want to dive into too deeply, so falling back on the non-Services.jsm version:
https://tbpl.mozilla.org/?tree=Try&rev=c1ba53424c2c
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c1ba53424c2c
Still seeing test-timeouts that are don't reproduce locally, pushed with small changes and some more tracing:
https://tbpl.mozilla.org/?tree=Try&rev=1f5a4f6fc425
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=1f5a4f6fc425
One fix and more tracing for strange unexpected OpenH264Provider.findUpdates() calls:
https://tbpl.mozilla.org/?tree=Try&rev=0733010bde0e
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=0733010bde0e
Workaround & more logging to find previous test that triggers:
https://tbpl.mozilla.org/?tree=Try&rev=4a48c34f57cb
Looks like this just came down to long timeouts on dummy https URLs, using http now:
https://tbpl.mozilla.org/?tree=Try&rev=887b8c61639f
https://hg.mozilla.org/mozilla-central/rev/1a9b83ef50a0
https://hg.mozilla.org/mozilla-central/rev/bfab1b77459d
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
(In reply to Georg Fritzsche [:gfritzsche] from comment #24)
> Looks like this just came down to long timeouts on dummy https URLs, using

Please could you also update the talos pref? :-)
https://hg.mozilla.org/build/talos/file/a418788b2dce/talos/PerfConfigurator.py#l301
Flags: needinfo?(georg.fritzsche)
Attachment #8470802 - Flags: review?(emorley)
Flags: needinfo?(georg.fritzsche)
Comment on attachment 8470802 [details] [diff] [review]
Fix Talos dummy URL

Thank you :-)
Attachment #8470802 - Flags: review?(emorley) → review+
QA Contact: drno → florin.mezei
QA Contact: florin.mezei → alexandra.lucinet
I tested using latest Nightly on Windows 7 64bit, Ubuntu 14.04 32bit and Mac OS X 10.9.4, the message 'OpenH264Wrapper::findUpdates() - last check was less then a day ago' appears on all platforms, using steps from comment 11.
Status: RESOLVED → VERIFIED
QA Whiteboard: [qa+] → [qa!]
georg: can you propose this for Fx 33?
Flags: needinfo?(georg.fritzsche)
Comment on attachment 8467160 [details] [diff] [review]
Handle UPDATE_WHEN_PERIODIC_UPDATE

Approval Request Comment
[Feature/regressing bug #]: OpenH264 integration
[User impact if declined]: No OpenH264 updates during long-running browser sessions.
[Describe test coverage new/current, TBPL]: QA verification, automated tests
[Risks and why]: Low-risk, this just adds an additional update-check path for the AddonManagers periodic checks.
[String/UUID change made/needed]: None.
Attachment #8467160 - Flags: approval-mozilla-aurora?
Flags: needinfo?(georg.fritzsche)
Comment on attachment 8467172 [details] [diff] [review]
Set dummy URL for GMP updates

Approval Request Comment
Same as above, this just adds a test fix.
Attachment #8467172 - Flags: approval-mozilla-aurora?
Attachment #8467160 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8467172 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [openh264-uplift]
Verified fixed with latest Aurora 33.0a2 (Build ID: 20140831004002) on Windows 7 64-bit, Mac OS X 10.9.4 and Ubuntu 14.04 32-bit - with str from comment 11, the "OpenH264Wrapper::findUpdates() - last check was less then a day ago" message is properly displayed.
Based on this results and comment 31, changing the tracking flags accordingly.
You need to log in before you can comment on or make changes to this bug.