Drop down menus not working correctly with privacy.resistFingerprinting True setting and multi-process enabled

RESOLVED FIXED in Firefox 52

Status

()

Core
DOM: Core & HTML
P3
major
RESOLVED FIXED
9 months ago
5 months ago

People

(Reporter: b1883900, Assigned: bz)

Tracking

(Blocks: 1 bug)

50 Branch
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52- fixed, firefox53 fixed)

Details

Attachments

(7 attachments, 5 obsolete attachments)

24.79 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
6.18 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
2.76 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
5.12 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
9.58 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
1.93 KB, patch
smaug
: review+
Details | Diff | Splinter Review
10.14 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

9 months ago
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:50.0) Gecko/20100101 Firefox/50.0
Build ID: 20100101

Steps to reproduce:

This issue only stated after Firefox enabled multi-processes.  It wasn't present on this same version until multi-processes was enabled (automatically by Firefox, not manually).

1. Use Firefox multi-process enabled
2. Create/set privacy.resistFingerprinting to True in about:config (Add new preference, Boolean, privacy.resistFingerprinting, True)
3. Click on a drop down menu arrow.  For example, the 'Product' drop down menu at https://bugzilla.mozilla.org/query.cgi?format=specific.  


Actual results:

With privacy.resistFingerprinting set to True, drop down menus appear to register the initial click on the drop down menu arrow as also immediately then registering a click on whatever menu selection happens to open directly where the initial click registers.

For example, https://bugzilla.mozilla.org/query.cgi?format=specific.  Clicking on the drop down menu arrow next to 'Product' immediately selects either 'Calendar' or 'Android Background Services' (depending on mouse position on initial click of drop down menu arrow).  The only way to get them to work correctly is to click and hold until the mouse can be moved out of the way to a selection.

With privacy.resistFingerprinting set to False, they behave as normal.

This is not related to a mouse issue or present within the OS or Firefox UI in general.


Expected results:

With privacy.resistFingerprinting set to True, drop down menus should behave as they do with With privacy.resistFingerprinting set to False (unless this is expected behaivor due to this setting).
(Reporter)

Updated

9 months ago
Component: Untriaged → Preferences

Updated

9 months ago
Severity: normal → major
Status: UNCONFIRMED → NEW
Has STR: --- → yes
Component: Preferences → Untriaged
Ever confirmed: true
OS: Unspecified → All
Product: Firefox → Core
Hardware: Unspecified → All
Component: Untriaged → DOM: Core & HTML
Neil, do you know what could be going on here? (Is this the right component?)
Flags: needinfo?(enndeakin)
Blocks: 1207306
Possibly the fingerprint-resisting code is blocking us from getting the proper coordinates or something.
Flags: needinfo?(enndeakin)
Maybe Boris knows since he wrote some of the code around http://searchfox.org/mozilla-central/source/dom/events/MouseEvent.cpp#368 which checks for privacy.resistFingerprinting.
Flags: needinfo?(bzbarsky)
Priority: -- → P3
I just changed existing code a bit to do its "system caller" checks differently.  My changes are only in 53, but this bug is reported against 50.

Anyway, this is definitely a coordinate-getting issue.  If I set this pref in 50, the dropdown is shown at the exact top/left of the browser content area, not under/over the combobox as it should be.  On tip it does the same thing.  In both cases this can cause the dropdown to overlap the combobox and hence be under the mouse.  On 50, this causes something to get selected; on tip that part is fixed (at least on Mac) by the patch that was landed for bug 1316597, I think, because bug 430745 made it so the dropdown always overlaps the combobox.

Anyway, the key part is that the dropdown is being placed in the wrong place.
Flags: needinfo?(bzbarsky)
See Also: → bug 1308340
A quick stop in the debugger says that resource://gre/modules/BrowserUtils.jsm calls getElementBoundingScreenRect which calls getElementBoundingRect which calls into nsGlobalWindow::GetMozInnerScreenX.  This calls nsContentUtils::ShouldResistFingerprinting which makes the decision purely based on whether the window's docshell is chrome or not, not based on the caller.  This means that chrome code can't get accurate coordinates for things in content docshells using these APIs.

I have no idea why this bit is not taking the caller into account; needinfoing the author and reviewers of this code.
Blocks: 418986
Flags: needinfo?(mrbkap)
Flags: needinfo?(cam)
Flags: needinfo?(arthuredelstein)
For what it's worth, I expect that the right fix here is to change all nsGlobalWindow callers of nsContentUtils::ShouldResistFingerprinting to checks more like the ones in MouseEvent: check the caller type, and if not system call nsContentUtils::ResistFingerprinting().  In fact, ideally we'd just push the CallerType down into ResistFingerPrinting....
> which makes the decision purely based on whether the window's docshell is chrome or not

Correction, based on whether the window's docshell's current document is system-principal.
> In fact, ideally we'd just push the CallerType down into ResistFingerPrinting....

I'm doing that in bug 1324035.
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #5)
> A quick stop in the debugger says that
> resource://gre/modules/BrowserUtils.jsm calls getElementBoundingScreenRect
> which calls getElementBoundingRect which calls into
> nsGlobalWindow::GetMozInnerScreenX.  This calls
> nsContentUtils::ShouldResistFingerprinting which makes the decision purely
> based on whether the window's docshell is chrome or not, not based on the
> caller.  This means that chrome code can't get accurate coordinates for
> things in content docshells using these APIs.
> 
> I have no idea why this bit is not taking the caller into account;
> needinfoing the author and reviewers of this code.

Hi Boris -- I think what you are saying makes sense and using the caller in nsContentUtils::ShouldResistFingerprinting would be a great idea. We would just need to be very careful that changing it this way doesn't unexpectedly leak forbidden data to content scripts. But the existing unit tests goes some way to ensuring that.
Flags: needinfo?(arthuredelstein)
With bug 1324035 landed, is this now fixed?
Flags: needinfo?(cam) → needinfo?(arthuredelstein)
(In reply to Cameron McCormack (:heycam) from comment #10)
> With bug 1324035 landed, is this now fixed?

Yes for nsMimeTypeArray and nsPluginArray, but not in general. A number of calls to ShouldResistFingerpritning remain that are using the document type instead of the caller type:

https://dxr.mozilla.org/mozilla-central/search?q=%2Bcallers%3A%22nsContentUtils%3A%3AShouldResistFingerprinting(nsIDocShell+*)%22
Flags: needinfo?(arthuredelstein)
Created attachment 8820608 [details] [diff] [review]
part 1.  Switch from ShouldResistFingerprinting to ResistFingerprinting (use the caller type, not the document principal) in nsGlobalWindow's outerWidth/outerHeight getters
Attachment #8820608 - Flags: review?(bkelly)
(Assignee)

Updated

8 months ago
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Created attachment 8820609 [details] [diff] [review]
part 2.  Switch from ShouldResistFingerprinting to ResistFingerprinting (use the caller type, not the document principal) in nsGlobalWindow's screenX/screenY getters
Attachment #8820609 - Flags: review?(bkelly)
Created attachment 8820610 [details] [diff] [review]
part 3.  Switch from ShouldResistFingerprinting to ResistFingerprinting (use the caller type, not the document principal) in nsGlobalWindow's orientation getter
Attachment #8820610 - Flags: review?(bkelly)
Created attachment 8820611 [details] [diff] [review]
part 4.  Switch from ShouldResistFingerprinting to ResistFingerprinting (use the caller type, not the document principal) in nsGlobalWindow's mozInnerScreenX/mozInnerScreenY getters
Attachment #8820611 - Flags: review?(bkelly)
Created attachment 8820613 [details] [diff] [review]
part 5.   Switch from ShouldResistFingerprinting to ResistFingerprinting (use the caller type, not the document principal) in nsGlobalWindow's devicePixelRatio getter
Attachment #8820613 - Flags: review?(bkelly)
Created attachment 8820623 [details] [diff] [review]
part 1.  Remove some unused nsIDOMScreen function
Attachment #8820623 - Flags: review?(bkelly)
Created attachment 8820624 [details] [diff] [review]
part 2.  Switch ScreenOrientation's deviceType getter to use the caller-type version of ResistFingerprinting
Attachment #8820624 - Flags: review?(bkelly)
Created attachment 8820625 [details] [diff] [review]
part 3.  Switch ScreenOrientation's DeviceAngle getter to use the caller-type version of ResistFingerprinting
Attachment #8820625 - Flags: review?(bkelly)
Created attachment 8820626 [details] [diff] [review]
part 4.  Switch ScreenOrientation's type getter to use the caller-type version of ResistFingerprinting
Attachment #8820626 - Flags: review?(bkelly)
Created attachment 8820628 [details] [diff] [review]
part 5.  Switch ScreenOrientation's angle getter to use the caller-type version of ResistFingerprinting
Attachment #8820628 - Flags: review?(bkelly)
Comment on attachment 8820623 [details] [diff] [review]
part 1.  Remove some unused nsIDOMScreen function

Er, wrong bug#.
Attachment #8820623 - Attachment is obsolete: true
Attachment #8820623 - Flags: review?(bkelly)
(Assignee)

Updated

8 months ago
Attachment #8820624 - Attachment is obsolete: true
Attachment #8820624 - Flags: review?(bkelly)
(Assignee)

Updated

8 months ago
Flags: needinfo?(mrbkap)
(Assignee)

Updated

8 months ago
Attachment #8820625 - Attachment is obsolete: true
Attachment #8820625 - Flags: review?(bkelly)
(Assignee)

Updated

8 months ago
Attachment #8820626 - Attachment is obsolete: true
Attachment #8820626 - Flags: review?(bkelly)
(Assignee)

Updated

8 months ago
Attachment #8820628 - Attachment is obsolete: true
Attachment #8820628 - Flags: review?(bkelly)
(Assignee)

Updated

8 months ago
Blocks: 1325028

Updated

8 months ago
Attachment #8820608 - Flags: review?(bkelly) → review+

Updated

8 months ago
Attachment #8820609 - Flags: review?(bkelly) → review+

Updated

8 months ago
Attachment #8820610 - Flags: review?(bkelly) → review+

Updated

8 months ago
Attachment #8820611 - Flags: review?(bkelly) → review+

Updated

8 months ago
Attachment #8820613 - Flags: review?(bkelly) → review+

Comment 23

8 months ago
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c179321130bf
part 1.  Switch from ShouldResistFingerprinting to ResistFingerprinting (use the caller type, not the document principal) in nsGlobalWindow's outerWidth/outerHeight getters.  r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a2f77e6666c
part 2.  Switch from ShouldResistFingerprinting to ResistFingerprinting (use the caller type, not the document principal) in nsGlobalWindow's screenX/screenY getters.  r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2d71a693532
part 3.  Switch from ShouldResistFingerprinting to ResistFingerprinting (use the caller type, not the document principal) in nsGlobalWindow's orientation getter.  r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/08a47e50686b
part 4.  Switch from ShouldResistFingerprinting to ResistFingerprinting (use the caller type, not the document principal) in nsGlobalWindow's mozInnerScreenX/mozInnerScreenY getters.  r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/1098e5ab12de
part 5.   Switch from ShouldResistFingerprinting to ResistFingerprinting (use the caller type, not the document principal) in nsGlobalWindow's devicePixelRatio getter.  r=bkelly

Comment 24

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c179321130bf
https://hg.mozilla.org/mozilla-central/rev/9a2f77e6666c
https://hg.mozilla.org/mozilla-central/rev/f2d71a693532
https://hg.mozilla.org/mozilla-central/rev/08a47e50686b
https://hg.mozilla.org/mozilla-central/rev/1098e5ab12de
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
(Reporter)

Comment 25

8 months ago
Will this fix make it into version 51 also or not until version 53 (meaning April for offical release users)?
I would be somewhat uncomfortable uplifting this to 51.  I might be convinced to try uplifting this to 52, though, if this is a serious problem in practice.
[Tracking Requested - why for this release]:
Tor Browser will be using this patch in 52ESR, so I would like to request uplifting to 52.
status-firefox52: --- → affected
tracking-firefox52: --- → ?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Please don't reopen bugs to request backports.  That's what patch approval flags are for.

I'm not entirely sure about how comfortable I am backporting this at this late date.  :(  I _think_ none of these patches change any behavior when fingerprinting resistance is not enabled, but I'm not sufficiently sure of it to bet my life on it, say.

Doing the backport three weeks ago, at the time of comment 26 might have been ok, but now we're a week away from 52 being beta...  I wish this had been seriously brought up then.
Status: REOPENED → RESOLVED
Last Resolved: 8 months ago7 months ago
Resolution: --- → FIXED
Not tracking for 52.  Based on comment 28 uplifting may be risky.
status-firefox52: affected → fix-optional
tracking-firefox52: ? → -
Oh, and the patches as written depend on the patches for bug 1324035 a bit.  We'd need to backport part of that too.
Luckily, they don't depend on much of it, because backporting that would be a pain.
Created attachment 8827750 [details] [diff] [review]
Prerequisite for making the rest of these patches compile on Aurora.  This should land before "Part 1".
Attachment #8827750 - Flags: review?(bugs)
Created attachment 8827751 [details] [diff] [review]
Part 5 backported to Aurora

The other parts apply just fine
Comment on attachment 8827750 [details] [diff] [review]
Prerequisite for making the rest of these patches compile on Aurora.  This should land before "Part 1".

Approval Request Comment
[Feature/Bug causing the regression]: Bug 418986
[User impact if declined]: Various dropdown menus will work incorrectly (appear
   in the wrong places, disappear when they should not) when the fingerprinting
   protection preference is turned on.
[Is this code covered by automated tests?]: Yes, but not with this preference
   enabled.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: Might not be a bad
   idea.  The steps in comment 0 are pretty good.
[List of other uplifts needed for the feature/fix]: With this bit added, this is
   self-contained.
[Is the change risky?]: I think if the fingerprinting preference is _not_
   enabled (the default state) the risk is quite low.  I did double-check that
   those codepaths are not affected by this patch.  If the preference _is_
   enabled, the risk is still fairly low, but now we do have a behavior change,
   which is always riskier than no change.
[Why is the change risky/not risky?]:  See above.
[String changes made/needed]: None.
Attachment #8827750 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

7 months ago
Attachment #8820608 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

7 months ago
Attachment #8820609 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

7 months ago
Attachment #8820610 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

7 months ago
Attachment #8820611 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

7 months ago
Attachment #8827751 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

7 months ago
Attachment #8827750 - Attachment description: Prerequisite for making the rest of these patches compile on Aurora → Prerequisite for making the rest of these patches compile on Aurora. This should land before "Part 1".

Updated

7 months ago
Attachment #8827750 - Flags: review?(bugs) → review+
Comment on attachment 8827750 [details] [diff] [review]
Prerequisite for making the rest of these patches compile on Aurora.  This should land before "Part 1".

fix menus with fingerprinting resistance enabled, let's get this into aurora52 before next week's merge
Attachment #8827750 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8820608 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8820609 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8820610 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8820611 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8827751 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 36

7 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/e8bc61cac015
https://hg.mozilla.org/releases/mozilla-aurora/rev/a8545918ab5f
https://hg.mozilla.org/releases/mozilla-aurora/rev/fe78462dc771
https://hg.mozilla.org/releases/mozilla-aurora/rev/cbc9396c381b
https://hg.mozilla.org/releases/mozilla-aurora/rev/e0b56aa9856f
https://hg.mozilla.org/releases/mozilla-aurora/rev/e93ab7604754
status-firefox52: fix-optional → fixed
You need to log in before you can comment on or make changes to this bug.