proposed change for #402 #407

ShijunS · 2016-10-06T18:15:51Z

the first attempt to fix #402

jan-ivar

WFM with some changes, primarily to not lose the permission requirement outright, except in the active tab with focus. I've added language suggestions

jan-ivar · 2016-10-06T18:35:54Z

getusermedia.html

-      <p>When new media input and/or output devices are made available, and if either the
-      <a data-lt="retrieve the permission state">permission state</a> of the
-      "list-devices" permission is "granted" or any of the local devices are
-      attached to an active MediaStream in the current browsing context, then


This removes all permission requirements for the devicechange event, which I don't think we agreed to.

I think we need to keep this, and an exception for the active tab that has input focus. Something like:

", or the active document in the current browsing context is fully active and has focus, then"

@jan-ivar, I'll update this part based on your input.

I still haven't figured out a situation where protecting the devicechange event gives any real protection (see previous discussion). Until I see such a scenario, I am in favor of removing all coupling between the devicechange event and the deviceinfo permission.

@alvestrand arguing for going further than this PR, is not a reason to hold up this PR I think.

This PR removes permission for the active tab and aligns enumerateDevices with the firing of the event, like we do with other events.

@alvestrand I would be against going further. If I have, say, 100 tabs open across 3 separate brand browsers, and pull out my USB device, and we signal all tabs about that immediately, that's a fairly strong timing signal to all those tabs' JS that I might be the same user, in spite of every other precaution I might have taken to be anonymous (including using separate browsers, VPNs, denying cookies, and any tracking-protecting private browsing modes).

I should add virtual machines to that list, if they have USB hooked up.

jan-ivar · 2016-10-06T19:12:02Z

getusermedia.html

+      the User Agent MUST run the following steps:</p>
+      <ol>
+        <li>
+          <p>If <var>storedDeviceList</var> exists, then delete <var>storedDeviceList</var>.</p>


Needs to be a [[storedDeviceList]] internal slot on the global object in the current realm I think, and set to an empty array.

@jan-ivar, thought about that, an "empty" list is still a valid list, i.e. before the event is fired.

@ShijunS Good point. Let's initialize it to undefined then, as "existence" can be ambiguous.

jan-ivar · 2016-10-06T19:15:42Z

getusermedia.html

@@ -3018,6 +3023,10 @@
                  <p>Run the following steps in parallel:</p>
                  <ol>
                    <li>
+                      <p>If <var>storedDeviceList</var> exists, then let <var>resultList</var> 


[[storedDeviceList]] is not undefined,

jan-ivar · 2016-10-06T19:18:51Z

getusermedia.html

@@ -3018,6 +3023,10 @@
                  <p>Run the following steps in parallel:</p>
                  <ol>
                    <li>
+                      <p>If <var>storedDeviceList</var> exists, then let <var>resultList</var> 
+                        be a copy of <var>storedDeviceList</var>, and skip steps 2-5 below.</p>


Since step 5 sets storedDeviceList to resultList, I think we can invert this condition and say:

"If [[storedDeviceList]] is empty, then execute the following steps:"

and move steps 2-5 in to be conditional sub-steps this step which culminate in setting [[storedDeviceList]].

@jan-ivar, if storedDeviceList exists, we still need to copy storedDeviceList to resultList. We might need more changes if we want to move 2-5 to sub-steps.

Or s/resultList/storedDeviceList/ in step 8 and 9.

I would prefer not to have explicit step numbers in the algorithms, since they need to be updated when a new step is added. You could either use a label and jump to it (see Permission Failure: in the gUM() steps), or restructure as @jan-ivar suggests.

Thanks @adam-be for the feedback. I made a few more edits, PTAL.

@jan-ivar

update based on input from @jan-ivar

@jan-ivar

as suggested by @jan-ivar, replacing "exists" with "is not undefined"

jan-ivar · 2016-10-07T21:14:59Z

getusermedia.html

      the User Agent MUST run the following steps:</p>
      <ol>
        <li>
-          <p>If <var>storedDeviceList</var> exists, then delete <var>storedDeviceList</var>.</p>
+          <p>If <var>storedDeviceList</var> is not undefined, then delete <var>storedDeviceList</var>


The scope of storedDeviceList isn't clear here IMHO. Usually, var refers to local vars made in-algorithm with let.

I still think it needs to be an internal slot, which we now have several of.

@jan-ivar, not sure if that is absolutely needed here given storedDeviceList is not tied directly to any JavaScript object. And, not sure how to reset an internal slot to undefined either. Let's get comment from other experts, @adam-be?

What is it tied to then? As I understand it, an internal slot can be set to undefined just like any other value.

If we need to tie this to an object, the MediaDevices will be the best match. @jan-ivar, any suggestion on at what point an internal slot could be allocated for MediaDevices?

Yes, I put mine on page load here.

@jan-ivar, please take another look with the new commit. I also replaced "undefined" with "null" to indicate the absence of a usable value.

@jan-ivar

as suggested by @jan-ivar.

jan-ivar

Great! I'd still flip the condition and inline steps as I mentioned earlier, but let's see what @adam-be says.

jan-ivar · 2016-10-11T04:20:45Z

getusermedia.html

-          <p>If <var>storedDeviceList</var> is not undefined, then delete <var>storedDeviceList</var>
-          so it becomes undefined.</p>
+          <p>If [[\storedDeviceList]] is not null, then set [[\storedDeviceList]]
+          to null.</p>


Or maybe just: "Set [[storedDeviceList]] to null." ?

jan-ivar · 2016-10-11T04:22:34Z

getusermedia.html

@@ -3093,7 +3099,7 @@
                      </ol>
                    </li>
                    <li>
-                      <p>Update <var>storedDeviceList</var> to be same as 
+                      <p>Update [[\storedDeviceList]]  to be same as 


Or maybe: "set [[storedDeviceList]] to resultList." ?

adam-be · 2016-10-13T10:50:47Z

getusermedia.html

+          relevant global object</a>, run the following steps:</p>
+          <ol>
+            <li>
+              <p>create three internal slots: [[\devicesLiveMap]], [[\devicesAccessibleMap]], and


nit: s/create/Create/

adam-be · 2016-10-13T10:52:24Z

getusermedia.html

-      <code><a>MediaDevices</a></code> object. The User Agent MAY fire just a
+      <p>When new media input and/or output devices are made available, if either the 
+      <a data-lt="retrieve the permission state">permission state</a> of the 
+      "list-devices" permission is "granted", or any of the local devices are 


#410 replaces "list-devices" with "device-info". I don't know which will be merged first, but we should make sure it's not forgotten.

#410 is merged now. Please update this PR accordingly.

s/list-devices/device-info/ s/create/Create replace step numbers with a label

stefhak · 2016-10-20T08:42:48Z

This LGTM now. Time to merge?

stefhak · 2016-10-20T14:11:18Z

@ShijunS can you resolve the conflicts? We'll then merge.

ShijunS · 2016-10-21T21:21:58Z

I had problem when resolving merge conflict in this PR patch, so replacing with PR #412.

proposed change for #402

b37e0ea

the first attempt to fix #402

jan-ivar reviewed Oct 6, 2016

View reviewed changes

ShijunS added 2 commits October 6, 2016 13:31

proposed change for #402

d5e8ffb

update based on input from @jan-ivar

proposed change for #402

387ca28

as suggested by @jan-ivar, replacing "exists" with "is not undefined"

jan-ivar reviewed Oct 7, 2016

View reviewed changes

change storedDeviceList to an internal slot

86616df

as suggested by @jan-ivar.

jan-ivar reviewed Oct 11, 2016

View reviewed changes

make [[\storedDeviceList]] operations more concise

407ef8c

adam-be reviewed Oct 13, 2016

View reviewed changes

edits based on input from @adam-be

2cb0bad

s/list-devices/device-info/ s/create/Create replace step numbers with a label

ShijunS mentioned this pull request Oct 21, 2016

Rules for when devicechange events may fire #412

Merged

ShijunS closed this Oct 21, 2016

jan-ivar mentioned this pull request Dec 12, 2016

Polling enumerateDevices potentially being a fingerprint. #403

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

proposed change for #402 #407

proposed change for #402 #407

ShijunS commented Oct 6, 2016

jan-ivar left a comment

jan-ivar Oct 6, 2016 •

edited

Loading

ShijunS Oct 6, 2016

alvestrand Oct 14, 2016

jan-ivar Oct 14, 2016 •

edited

Loading

jan-ivar Oct 14, 2016

jan-ivar Oct 14, 2016

jan-ivar Oct 6, 2016 •

edited

Loading

ShijunS Oct 6, 2016

jan-ivar Oct 7, 2016

jan-ivar Oct 6, 2016 •

edited

Loading

jan-ivar Oct 6, 2016 •

edited

Loading

ShijunS Oct 6, 2016

jan-ivar Oct 7, 2016 •

edited

Loading

adam-be Oct 13, 2016

ShijunS Oct 13, 2016

jan-ivar Oct 7, 2016

ShijunS Oct 7, 2016

jan-ivar Oct 10, 2016

ShijunS Oct 10, 2016

jan-ivar Oct 10, 2016

ShijunS Oct 10, 2016

jan-ivar left a comment

jan-ivar Oct 11, 2016

jan-ivar Oct 11, 2016

adam-be Oct 13, 2016

adam-be Oct 13, 2016

adam-be Oct 13, 2016

stefhak commented Oct 20, 2016

stefhak commented Oct 20, 2016

ShijunS commented Oct 21, 2016

proposed change for #402 #407

proposed change for #402 #407

Conversation

ShijunS commented Oct 6, 2016

jan-ivar left a comment

Choose a reason for hiding this comment

jan-ivar Oct 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jan-ivar Oct 14, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jan-ivar Oct 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jan-ivar Oct 6, 2016 • edited Loading

Choose a reason for hiding this comment

jan-ivar Oct 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jan-ivar Oct 7, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jan-ivar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefhak commented Oct 20, 2016

stefhak commented Oct 20, 2016

ShijunS commented Oct 21, 2016

jan-ivar Oct 6, 2016 •

edited

Loading

jan-ivar Oct 14, 2016 •

edited

Loading

jan-ivar Oct 6, 2016 •

edited

Loading

jan-ivar Oct 6, 2016 •

edited

Loading

jan-ivar Oct 6, 2016 •

edited

Loading

jan-ivar Oct 7, 2016 •

edited

Loading