The OpenH264 path should be relative to the profile directory and include a version subdirectory

VERIFIED FIXED in Firefox 33

Status

()

Toolkit
Add-ons Manager
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: johnp, Assigned: qeole, Mentored)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla34
Points:
5
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +
qe-verify +

Firefox Tracking Flags

(firefox32 unaffected, firefox33+ verified, firefox34 verified)

Details

(Whiteboard: [lang=js] [diamond])

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

4 years ago
After moving my whole system to an SSD which previously only contained firefox in D:\Program Files\Nightly\ with the profile symlinked from %appdata%/... to D:\Program Files\Nightly\profile, the media.gmp-gmpopenh264.path config entry is now still at at D:\Program Files\Nightly\profile\gmp-gmpopenh264, altough firefox and it's profile are now in their default locations on C:.

Shouldn't that path be relative to the profile path and if not, isn't a hard-coded path were a .dll is loaded from and that can probably be changed by another application a security issue? Could the path be changed to point to a malicious .dll?
(Reporter)

Updated

4 years ago
Blocks: 948160
Hardware: x86 → x86_64

Updated

4 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Ok, that's a shortcoming in our solution - relative to profile dir sounds workable. But given that moving profiles around is not common it should not block.

I don't see a security bug here though - if you can change the pref and drop binaries on the disk, you already have access.
Apparently i can't unset the sec flag though.
Flags: firefox-backlog+
OS: Windows 8.1 → All
Hardware: x86_64 → All
(Reporter)

Comment 2

4 years ago
(In reply to Georg Fritzsche [:gfritzsche] from comment #1)
Sounds reasonable. Just wanted to make sure with all those DLL hijacking issues over the course of the last years that this couldn't be used maliciously.
Georg -- I think this is in the Add-ons manager.  Can you take this bug or find someone to take it?
Group: core-security
Component: WebRTC: Audio/Video → Add-ons Manager
Flags: needinfo?(georg.fritzsche)
Product: Core → Toolkit
I put it on our Firefox backlog, so it should get picked up at some point. The question is whether we can live with it for now or not (and the prioritization over e.g. my other tasks).
Flags: needinfo?(georg.fritzsche)
I think the priority depends on how likely it is for the average user to experience this problem.  Based on the bug description and comments, it seems pretty unlikely. So we can probably make this non-blocking.  Do you agree?  (Does anyone disagree?)
Flags: needinfo?(georg.fritzsche)
(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #5)
> Based on the bug description and comments, it
> seems pretty unlikely. So we can probably make this non-blocking.  Do you
> agree?

I agree.
Flags: needinfo?(georg.fritzsche)
Talking with Benjamin this is more likely then we assumed - e.g. enterprise setups with roaming profiles would be affected (e.g. when mounted to different drives).
Marco, we need this in 33 - can we get this bumped up into the next iteration?
Flags: needinfo?(mmucci)
Forwarding 'needinfo' to Gavin for considering this bug in the upcoming Iteration 34.2

(In reply to Georg Fritzsche [:gfritzsche] from comment #8)
> Marco, we need this in 33 - can we get this bumped up into the next
> iteration?
Flags: needinfo?(mmucci) → needinfo?(gavin.sharp)
Let's add to the 34.2 backlog.
Flags: needinfo?(gavin.sharp)
Added to 34.2 Backlog.

(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #10)
> Let's add to the 34.2 backlog.
Hi Georg, can you provide a point value for this bug.
Flags: needinfo?(georg.fritzsche)
Points: --- → 5
Flags: needinfo?(georg.fritzsche)
bbondy, is this something you could potentially look into?
Flags: needinfo?(netzen)
We would have to set a path relative to the profile here:
http://hg.mozilla.org/mozilla-central/annotate/71497ed2e0db/toolkit/modules/GMPInstallManager.jsm#l885

... and expand it from |Services.dirsvc.get('ProfD', Ci.nsIFile)| everywhere OPENH264_PREF_PATH gets used in OpenH264Provider.jsm.
http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/OpenH264Provider.jsm
Mentor: georg.fritzsche
Whiteboard: [lang=js]
Adding uplift to the whiteboard based on Comment 7.
Whiteboard: [lang=js] → [lang=js][openh264-uplift]

Comment 16

4 years ago
I'm not convinced that we need to use a path for this at all. We already know that it's going to be in <profile>/..../<version> right? If so can we just store the current version in prefs?

i.e. s/media.gmp-gmpopenh264.path/media.gmp-gmpopenh264.currentVersion/
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #16)
> I'm not convinced that we need to use a path for this at all. We already
> know that it's going to be in <profile>/..../<version> right?

We already store the version ("media.gmp-gmpopenh264.version"), but use <profile>/<gmp-id>.
That sounds sensible though and we could easily switch to installing the GMP into a version subdir.
I think we can just use  <profile>/<gmp-id> and assume if it exists it'll be the latest no?
Flags: needinfo?(netzen)
I think that we can actually run into "update while in use issues" without this, right?
This would leave the "remove older versions" issue, but we could look into it in a follow-up.
Maybe on Windows, we could probably rename out of the way before upgrading though. And delete any old renamed file if one exists.  Not sure if that's more simple than the version thing though.

I'll probably just do the version dir thing and at the same time I'd check what the old version is and delete it if it exists.  I'll handle both in this bug.
QA Contact: netzen
Assignee: nobody → netzen
QA Contact: netzen
Oops, thanks for fixing.
Mentor: georg.fritzsche
Whiteboard: [lang=js][openh264-uplift] → [openh264-uplift]
Assignee: netzen → nobody
Mentor: georg.fritzsche
Whiteboard: [openh264-uplift] → [openh264-uplift] [lang=js] [diamond]
Summary: media.gmp-gmpopenh264.path does not update if profile is moved → The OpenH264 path should be relative to the profile directory and include a version subdirectory
* We won't need the media.gmp-gmpopenh264.path preference anymore (affects GMPInstallManager & OpenH264Provider)
* OpenH264Provider should listen to changes to the media.gmp-gmpopenh264.version pref instead
* the plugin will be installed into <profile-dir>/<gmp-plugin-id>/<version>
* some tests using the path property will need fixup (probably toolkit/mozapps/extensions/test/browser/browser_openH264.js & .../xpcshell/test_openH264.js)

Helpful:
* OS.Path.join(OS.Constants.Path.profileDir, ...)
* install path after download: http://hg.mozilla.org/mozilla-central/annotate/71497ed2e0db/toolkit/modules/GMPInstallManager.jsm#l872
[Tracking Requested - why for this release]:
status-firefox32: --- → unaffected
status-firefox33: --- → affected
status-firefox34: --- → affected
tracking-firefox33: --- → ?
Assignee: nobody → qeole
Tracking but we won't block the release for this.
tracking-firefox33: ? → +
(Assignee)

Comment 25

4 years ago
Created attachment 8472750 [details] [diff] [review]
Changing_install_dir_and_remove_path-preference.patch

(In reply to Georg Fritzsche [:gfritzsche] from comment #22)
> * We won't need the media.gmp-gmpopenh264.path preference anymore (affects
> GMPInstallManager & OpenH264Provider)
Also affects GMPParent.cpp [1] and GMPChild.cpp [2], in which we rely on plugin path to extract plugin name.
> * OpenH264Provider should listen to changes to the
> media.gmp-gmpopenh264.version pref instead
> * the plugin will be installed into <profile-dir>/<gmp-plugin-id>/<version>
I've addressed the three points above in enclosed patch.
|media.gmp-gmpopenh264.path| preference is gone, path is <profile-dir>/<gmp-plugin-id>/<version>, and plugin still loads correctly.

> * some tests using the path property will need fixup (probably
> toolkit/mozapps/extensions/test/browser/browser_openH264.js &
> .../xpcshell/test_openH264.js)
Tests will be for next week, assuming feedback is positive.

[1] https://hg.mozilla.org/mozilla-central/annotate/6a7be7376caa/content/media/gmp/GMPParent.cpp#l100
[2] https://hg.mozilla.org/mozilla-central/annotate/6a7be7376caa/content/media/gmp/GMPChild.cpp#l66
Attachment #8472750 - Flags: feedback?(georg.fritzsche)

Updated

4 years ago
QA Whiteboard: [qa?]
Comment on attachment 8472750 [details] [diff] [review]
Changing_install_dir_and_remove_path-preference.patch

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

Thanks, that looks good!
I wasn't aware of the GMPChild/GMPParent dependencies here - that seems unfortunate when they could just receive a path instead of re-implementing the same logic - i'll file a new bug on that.

::: toolkit/mozapps/extensions/internal/OpenH264Provider.jsm
@@ +218,5 @@
>    get pluginMimeTypes() { return []; },
>    get pluginLibraries() {
> +    if (this.isInstalled) {
> +      let path = this.version;
> +      return [path];

This will probably look a little awkward in about:plugins, but we're not exposing this in "normal" UI anywhere, so i don't think that's a real concern now.

@@ +300,5 @@
>      let wrapper = OpenH264Wrapper;
>  
>      AddonManagerPrivate.callAddonListeners("onUninstalling", wrapper, false);
>      if (this.gmpPath) {
> +      this._log.info("onPrefVersionChanged() - removing gmp directory " + this.gmpPath);

Can we change this to "unregistering gmp directory" while we're here?
Reading through this i realize that easy to misunderstand.

@@ +316,2 @@
>      if (this.gmpPath && wrapper.isActive) {
> +      this._log.info("onPrefVersionChanged() - adding gmp directory " + this.gmpPath);

"registering gmp directory"?
Attachment #8472750 - Flags: feedback?(georg.fritzsche) → feedback+
(In reply to Georg Fritzsche [:gfritzsche] from comment #26)
> I wasn't aware of the GMPChild/GMPParent dependencies here - that seems
> unfortunate when they could just receive a path instead of re-implementing
> the same logic - i'll file a new bug on that.

I meant "receive the plugin id".
QA Whiteboard: [qa?] → [qa+]

Updated

4 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 28

4 years ago
(In reply to Georg Fritzsche [:gfritzsche] from comment #26)
> I wasn't aware of the GMPChild/GMPParent dependencies here - that seems
> unfortunate when they could just receive a path instead of re-implementing
> the same logic - i'll file a new bug on that.

I saw you filled bug 1053727. Patch from bug 1040905 (rebased and waiting for review) will modify same portion of code, should it be worth setting it blocking for bug 1053727?
 
> ::: toolkit/mozapps/extensions/internal/OpenH264Provider.jsm
> @@ +218,5 @@
> >    get pluginMimeTypes() { return []; },
> >    get pluginLibraries() {
> > +    if (this.isInstalled) {
> > +      let path = this.version;
> > +      return [path];
> 
> This will probably look a little awkward in about:plugins, but we're not
> exposing this in "normal" UI anywhere, so i don't think that's a real
> concern now.

What would you suggest instead?

> […]
> Can we change this to…

Will do.
(In reply to qeole from comment #28)
> I saw you filled bug 1053727. Patch from bug 1040905 (rebased and waiting
> for review) will modify same portion of code, should it be worth setting it
> blocking for bug 1053727?

Right, done.

> > ::: toolkit/mozapps/extensions/internal/OpenH264Provider.jsm
> > @@ +218,5 @@
> > >    get pluginMimeTypes() { return []; },
> > >    get pluginLibraries() {
> > > +    if (this.isInstalled) {
> > > +      let path = this.version;
> > > +      return [path];
> > 
> > This will probably look a little awkward in about:plugins, but we're not
> > exposing this in "normal" UI anywhere, so i don't think that's a real
> > concern now.
> 
> What would you suggest instead?

We could do something like [0] here, but given that this only seen in about:plugins for OpenH264 let's leave it for now.
(Assignee)

Comment 30

4 years ago
Created attachment 8474490 [details] [diff] [review]
Changing_install_dir_and_remove_path-preference.patch

Updating patch with modified comments as suggested by Georg in comment #26.
Asking review for said patch.
Attachment #8472750 - Attachment is obsolete: true
Attachment #8474490 - Flags: review?(georg.fritzsche)
(Assignee)

Comment 31

4 years ago
Created attachment 8474494 [details] [diff] [review]
Tests.patch

Here is a separate patch updating the tests (toolkit/mozapps/extensions/test/browser/browser_openH264.js and toolkit/mozapps/extensions/test/xpcshell/test_openh264.js).

* browser_openH264.js
I had to correct some wrong |Services.prefs.setBoolPref()| calls to |Services.prefs.setCharPref()| for setting version and last update time.

* test_openh264.js
There would be some test to add regarding version handling for plugin update once this is implemented in plugin management (currently we don't check that newly registered plugin has higher version than former one). See FIXME tag in the patch. I believe it could be relevant to make those tests part of bug 1053729.

Both test files passed successfully on my computer.
Attachment #8474494 - Flags: feedback?(georg.fritzsche)
Attachment #8474490 - Flags: review?(georg.fritzsche) → feedback+
Comment on attachment 8474490 [details] [diff] [review]
Changing_install_dir_and_remove_path-preference.patch

r?jesup for GMPChild.cpp, GMPParent.cpp
r?unfocused for GMPInstallManager.jsm, OpenH264Provider.jsm
Attachment #8474490 - Flags: review?(rjesup)
Attachment #8474490 - Flags: review?(bmcbride)
Comment on attachment 8474494 [details] [diff] [review]
Tests.patch

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

This looks fine, with the below comment addressed you should go forward and push to try and request review from Unfocused (Blair).

::: toolkit/mozapps/extensions/test/xpcshell/test_openh264.js
@@ +216,5 @@
>    Assert.equal(removedPath, null);
>  
>    // Changing the pref mid-session should cause unregistration and registration.
> +  // FIXME: Need a test to verify that nothing happens if registered version number
> +  // is higher than unregistered plugin version number. See also bug 1053729.

I don't think we need to assert this here and can just remove the FIXME comment.
If our update/install handling picks something, we should accept it and not worry about version ordering.
If we don't have something like this, we could add checks for GMPInstallManager or the providers update picking in another bug.
Attachment #8474494 - Flags: feedback?(georg.fritzsche) → feedback+
(Assignee)

Comment 34

4 years ago
Created attachment 8474644 [details] [diff] [review]
Tests.patch

Updating patch related to tests.

There were some tests we had forgotten, here is the list of files now affected by the patch:

dom/media/gmp-plugin/Makefile.in
testing/mochitest/runtests.py
toolkit/modules/tests/xpcshell/test_GMPInstallManager.js
toolkit/mozapps/extensions/test/browser/browser_openH264.js
toolkit/mozapps/extensions/test/xpcshell/test_openh264.js

I've just made a push to try:
https://tbpl.mozilla.org/?tree=Try&rev=5b764964f42a
Attachment #8474494 - Attachment is obsolete: true
Attachment #8474644 - Flags: review?(bmcbride)
(Assignee)

Comment 35

4 years ago
I'm having issues with my push to try (https://tbpl.mozilla.org/?tree=Try&rev=5b764964f42a).
There's an error message preceded by “INFO -  Could not find path to gmp-fake plugin!”.

I think this message comes from runtests.py file, but I can't understand why we have it. There may be a place where I forgot to update plugin path, or I may have done something wrong in runtests.py? Or something else?

jesup, do you have any idea about that, please? Or about a way I could obtain more info?
Flags: needinfo?(rjesup)
ted did the runtests.py/gmp stuff - forwarding needinfo
Flags: needinfo?(rjesup) → needinfo?(ted)
Comment on attachment 8474490 [details] [diff] [review]
Changing_install_dir_and_remove_path-preference.patch

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

runtests.py and some other stuff will need to change...  r+ for the two files; r- for not dealing with the test automation  (r+ with a test patch that ted r+'d)
Attachment #8474490 - Flags: review?(rjesup) → review-
(Assignee)

Comment 38

4 years ago
Created attachment 8475063 [details] [diff] [review]
Tests.patch

Thank you jesup for review and needinfo forwarding.

(In reply to Randell Jesup [:jesup] from comment #37)

> runtests.py and some other stuff will need to change...
What do you mean exactly?

The error causing (most of) those oranges was due to fake plugin install commands in testing/mochitest/Makefile.in, and I fixed it (but I can't clear the needinfo request for Ted).

I ran another push to try but still have an issue on test bc3 for Windows, I'm investigating.
https://tbpl.mozilla.org/?tree=Try&rev=16a55783d0c6
Attachment #8474644 - Attachment is obsolete: true
Attachment #8474644 - Flags: review?(bmcbride)
Flags: needinfo?(rjesup)
Flags: needinfo?(ted)
(In reply to qeole from comment #38)
> (In reply to Randell Jesup [:jesup] from comment #37)
> 
> > runtests.py and some other stuff will need to change...
> What do you mean exactly?

I think jesup missed that the test changes are in a separate patch.
Flags: needinfo?(rjesup)
(Assignee)

Comment 40

4 years ago
I need some help with the oranges from last try push
(https://tbpl.mozilla.org/?tree=Try&rev=16a55783d0c6).

There's something happening for bc3 tests on Windows and I can't figure out what it is exactly.
There is probably something wrong happenig around [1].

It seems to me that there is a call to a logging function with "Too few arguments"; in particular, I added debug prints in [2], and could observe that |message| is a dictionary, and that it lacks a 'subtest' key entry (and associated value) for the line which seems to break the test. I'm not 100% sure it comes from there, but that's the impression I got. I have no idea how my patches could affect this 'subset' entry, though.

Would anyone have a hint here? jmaher, maybe?

[1] http://hg.mozilla.org/mozilla-central/file/4d94eeca89f3/toolkit/mozapps/extensions/test/browser/browser_openH264.js#l131
[2] http://hg.mozilla.org/mozilla-central/file/4d94eeca89f3/testing/mochitest/runtests.py#l1863
Flags: needinfo?(jmaher)
Chris, there is an error https://tbpl.mozilla.org/?tree=Try&rev=16a55783d0c6 that looks like this:
20:17:31     INFO -  4981 INFO TEST-PASS | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_openH264.js | Got add-on element. - [object XULElement] == true
20:17:31     INFO -  Exception in thread Thread-24:
20:17:31     INFO -  Traceback (most recent call last):
20:17:31     INFO -    File "c:\mozilla-build\python27\Lib\threading.py", line 551, in __bootstrap_inner
20:17:31     INFO -      self.run()
20:17:31     INFO -    File "c:\mozilla-build\python27\Lib\threading.py", line 504, in run
20:17:31     INFO -      self.__target(*self.__args, **self.__kwargs)
20:17:31     INFO -    File "C:\slave\test\build\venv\lib\site-packages\mozprocess\processhandler.py", line 775, in _processOutput
20:17:31     INFO -      self.processOutputLine(line.rstrip())
20:17:31     INFO -    File "C:\slave\test\build\venv\lib\site-packages\mozprocess\processhandler.py", line 720, in processOutputLine
20:17:31     INFO -      handler(line)
20:17:31     INFO -    File "C:\slave\test\build\tests\mochitest/runtests.py", line 1858, in processOutputLine
20:17:31     INFO -      self.harness.message_logger.process_message(message)
20:17:31     INFO -    File "C:\slave\test\build\tests\mochitest/runtests.py", line 178, in process_message
20:17:31     INFO -      self.dump_buffered(limit=True)
20:17:31     INFO -    File "C:\slave\test\build\tests\mochitest/runtests.py", line 202, in dump_buffered
20:17:31     INFO -      self.logger.log_raw(buf_msg)
20:17:31     INFO -    File "C:\slave\test\build\venv\lib\site-packages\mozlog\structured\structuredlog.py", line 142, in log_raw
20:17:31     INFO -      converted_data = convertor_registry[action].convert_known(**raw_data)
20:17:31     INFO -    File "C:\slave\test\build\venv\lib\site-packages\mozlog\structured\logtypes.py", line 96, in convert_known
20:17:31     INFO -      return self.convert(**known_kwargs)
20:17:31     INFO -    File "C:\slave\test\build\venv\lib\site-packages\mozlog\structured\logtypes.py", line 61, in convert
20:17:31     INFO -      raise TypeError("Too few arguments")
20:17:31     INFO -  TypeError: Too few arguments

Is there a chance that we now require a certain format for debug messages?
Flags: needinfo?(jmaher) → needinfo?(cmanchester)
(Assignee)

Comment 42

4 years ago
Created attachment 8475819 [details] [diff] [review]
Tests.patch

Well, it works now.
Thank you jmaher, and sorry for the bother (and once again, I can't clear NR for Chris).

I fixed a minor error I had made (I had turned |Services.prefs.setCharPref()| for |OPENH264_PREF_LASTUPDATE| to |setCharPrefs()| instead of |setIntPref()|), but what seemed to fix the above problem was mostly a source update.

Anyway, here is a successful try push:
https://tbpl.mozilla.org/?tree=Try&rev=0e75ec00060f

I'm now waiting for patch from bug 1040905 to be checked in to ask review here, as I'll most probably have to rebase code for GMPChild.cpp.
Attachment #8475063 - Attachment is obsolete: true
glad to see it is working now.
Flags: needinfo?(cmanchester)
(Assignee)

Comment 44

4 years ago
Comment on attachment 8474490 [details] [diff] [review]
Changing_install_dir_and_remove_path-preference.patch

Following Georg's suggestion on IRC, I'm asking review now :)

First patch (path handling):
r?jesup       content/media/gmp/GMPChild.cpp
r?jesup       content/media/gmp/GMPParent.cpp
r?unfocused   toolkit/modules/GMPInstallManager.jsm
r?unfocused   toolkit/mozapps/extensions/internal/OpenH264Provider.jsm

Second patch (tests):
r?jesup       dom/media/gmp-plugin/Makefile.in
r?jesup       testing/mochitest/Makefile.in
r?jmaher      testing/mochitest/runtests.py
r?unfocused   toolkit/modules/tests/xpcshell/test_GMPInstallManager.js
r?unfocused   toolkit/mozapps/extensions/test/browser/browser_openH264.js
r?unfocused   toolkit/mozapps/extensions/test/xpcshell/test_openh264.js
Attachment #8474490 - Flags: review- → review?(rjesup)
(Assignee)

Updated

4 years ago
Attachment #8475819 - Flags: review?(rjesup)
Attachment #8475819 - Flags: review?(jmaher)
Attachment #8475819 - Flags: review?(bmcbride)

Updated

4 years ago
Attachment #8474490 - Flags: review?(rjesup) → review+
Comment on attachment 8475819 [details] [diff] [review]
Tests.patch

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

Forwarding build-system review portion to ted
Attachment #8475819 - Flags: review?(rjesup) → review?(ted)
(Assignee)

Comment 46

4 years ago
Created attachment 8476254 [details] [diff] [review]
Changing_install_dir_and_remove_path-preference.patch

Patch for bug 1040905 has just been checked-in.
Rebasing first patch because of GMPChild.cpp file.
Attachment #8474490 - Attachment is obsolete: true
Attachment #8474490 - Flags: review?(bmcbride)
Attachment #8476254 - Flags: review?(bmcbride)
Comment on attachment 8475819 [details] [diff] [review]
Tests.patch

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

nothing looks concerning here, thanks for putting this together!
Attachment #8475819 - Flags: review?(jmaher) → review+
(Assignee)

Comment 48

4 years ago
Please note that I'm leaving today for two weeks or so. I won't be able to follow up or handle potential issues arising here before next uplift (September 1st).

Comment 49

4 years ago
Qeole, thank you for your help! Georg and I along with the media team will look after this and any regressions.
Comment on attachment 8475819 [details] [diff] [review]
Tests.patch

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

::: testing/mochitest/runtests.py
@@ +1162,5 @@
>      return manifest
>  
>    def getGMPPluginPath(self, options):
>      # For local builds, gmp-fake will be under dist/bin.
> +    gmp_path = os.path.join(options.xrePath, 'gmp-fake', '1.0')

n.b.: this will conflict with bug 1057328, and you'll have to fix the code path added there as well.
Attachment #8475819 - Flags: review?(ted) → review+
Hi Florin, can a QA contact be assigned for this bug.
QA Whiteboard: [qa+]
Flags: qe-verify+
Flags: needinfo?(florin.mezei)
Comment on attachment 8476254 [details] [diff] [review]
Changing_install_dir_and_remove_path-preference.patch

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

::: toolkit/modules/GMPInstallManager.jsm
@@ +899,5 @@
>          // Success, set the prefs
>          let now = Math.round(Date.now() / 1000);
>          GMPPrefs.set(GMPPrefs.KEY_ADDON_LAST_UPDATE, now, gmpAddon.id);
> +        // Setting the version pref signals installation completion to consumers,
> +        // so set potential other information they use first.

Nit: This is a bit confusing now there isn't any "other information" we're setting. Clarify?

::: toolkit/mozapps/extensions/internal/OpenH264Provider.jsm
@@ +225,2 @@
>    },
>    get pluginFullpath() {

You're not regressing this.... but I just realised both pluginFullpath and pluginLibraries are wrong, as they only point to the directory, not the file. Could you file a separate bug for this? (only need one)
Attachment #8476254 - Flags: review?(bmcbride) → review+
Comment on attachment 8475819 [details] [diff] [review]
Tests.patch

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

r+ with a couple of small fixes.

::: toolkit/modules/tests/xpcshell/test_GMPInstallManager.js
@@ -446,5 @@
>        // Make sure the prefs are set correctly
>        do_check_true(!!GMPPrefs.get(GMPPrefs.KEY_ADDON_LAST_UPDATE,
>                                     gmpAddon.id, ""));
> -      do_check_eq(GMPPrefs.get(GMPPrefs.KEY_ADDON_PATH, gmpAddon.id, ""),
> -                               extractedFile.parent.path);

By removing this, the test is no longer checking that the extracted file was placed where we expect it. Should add a check for extractedFile.path

@@ +447,5 @@
>        // Make sure it reports as being installed
>        do_check_true(gmpAddon.isInstalled);
>  
>        // Cleanup
>        extractedFile.parent.remove(true);

Need to update this too.
Attachment #8475819 - Flags: review?(bmcbride) → review+
Flags: needinfo?(florin.mezei)
QA Contact: alexandra.lucinet
(In reply to Blair McBride [:Unfocused] from comment #52)
> You're not regressing this.... but I just realised both pluginFullpath and
> pluginLibraries are wrong, as they only point to the directory, not the
> file. Could you file a separate bug for this? (only need one)

Right, i mentioned this above. I filed bug 1058042 now.
https://hg.mozilla.org/mozilla-central/rev/8058658b0a28
https://hg.mozilla.org/mozilla-central/rev/7c725f22a307
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Depends on: 1059011
Setting correct status flag.
status-firefox34: affected → fixed

Updated

4 years ago
Iteration: --- → 34.3
How is the verification going?
We want this uplifted, but should get it verified first.
Flags: needinfo?(alexandra.lucinet)

Comment 59

4 years ago
Reproduced with Nightly 2014-07-30
Verified as fixed with latest Nightly (Build ID: 20140828030205) on Windows 7 x64, Mac OS X 10.9.4 and Ubuntu 12.04 x32: after moving the profile, the plugin is displayed as installed and a h264 call is successfully performed.
No new issues found after performing some additional exploratory testing - all the logs are correctly generated; after resetting media.gmp-manager.lastCheck, a new addon check is correctly performed; crash reports are correctly generated.
Status: RESOLVED → VERIFIED
status-firefox34: fixed → verified
Flags: needinfo?(alexandra.lucinet)
Comment on attachment 8476254 [details] [diff] [review]
Changing_install_dir_and_remove_path-preference.patch

Approval Request Comment
[Feature/regressing bug #]: OpenH264 integration
[User impact if declined]: non-working OpenH264 after moving profile, e.g. in enterprise scenarios
[Describe test coverage new/current, TBPL]: we have mochitest & xpcshell coverage for the code involved, qa verified
[Risks and why]: lowish - it only touches path handling & pref handling in some well-understood locations and it is qa verified
[String/UUID change made/needed]: none
Attachment #8476254 - Flags: approval-mozilla-aurora?
Comment on attachment 8475819 [details] [diff] [review]
Tests.patch

Approval Request Comment
These are the required test changes for the above.
Attachment #8475819 - Flags: approval-mozilla-aurora?
Attachment #8475819 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8476254 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

4 years ago
Whiteboard: [openh264-uplift] [lang=js] [diamond] → [lang=js] [diamond]

Comment 64

4 years ago
Verified as fixed with latest Aurora 33.0a2 (Build ID: 20140901004002) on Windows 7 64-bit, Mac OS X 10.9.4 and Ubuntu 14.04 32-bit.
status-firefox33: fixed → verified
You need to log in before you can comment on or make changes to this bug.