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

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P3)

Description

[b1883900]

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).

Comment 1

[Andrew Overholt [:overholt]]

Neil, do you know what could be going on here? (Is this the right component?)

Comment 2

[Neil Deakin]

Possibly the fingerprint-resisting code is blocking us from getting the proper coordinates or something.

Comment 3

[Andrew Overholt [:overholt]]

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.

Comment 4

[Boris Zbarsky [:bzbarsky]]

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.

Comment 5

[Boris Zbarsky [:bzbarsky]]

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.

Comment 6

[Boris Zbarsky [:bzbarsky]]

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....

Comment 7

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> In fact, ideally we'd just push the CallerType down into ResistFingerPrinting....

I'm doing that in bug 1324035.

Comment 9

[Arthur Edelstein [:arthur]]

(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.

Comment 10

[Cameron McCormack (:heycam)]

With bug 1324035 landed, is this now fixed?

Comment 11

[Arthur Edelstein [:arthur]]

(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

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Switch from ShouldResistFingerprinting to ResistFingerprinting (use the caller type, not the document principal) in nsGlobalWindow's outerWidth/outerHeight getters

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Switch from ShouldResistFingerprinting to ResistFingerprinting (use the caller type, not the document principal) in nsGlobalWindow's screenX/screenY getters

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Switch from ShouldResistFingerprinting to ResistFingerprinting (use the caller type, not the document principal) in nsGlobalWindow's orientation getter

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Switch from ShouldResistFingerprinting to ResistFingerprinting (use the caller type, not the document principal) in nsGlobalWindow's mozInnerScreenX/mozInnerScreenY getters

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Switch from ShouldResistFingerprinting to ResistFingerprinting (use the caller type, not the document principal) in nsGlobalWindow's devicePixelRatio getter

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Remove some unused nsIDOMScreen function

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Switch ScreenOrientation's deviceType getter to use the caller-type version of ResistFingerprinting

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Switch ScreenOrientation's DeviceAngle getter to use the caller-type version of ResistFingerprinting

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Switch ScreenOrientation's type getter to use the caller-type version of ResistFingerprinting

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Switch ScreenOrientation's angle getter to use the caller-type version of ResistFingerprinting

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8820623 [details] [diff] [review]
part 1.  Remove some unused nsIDOMScreen function

Er, wrong bug#.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
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

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 25

[b1883900]

Will this fix make it into version 51 also or not until version 53 (meaning April for offical release users)?

Comment 26

[Boris Zbarsky [:bzbarsky]]

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.

Comment 27

[Arthur Edelstein [:arthur]]

[Tracking Requested - why for this release]:
Tor Browser will be using this patch in 52ESR, so I would like to request uplifting to 52.

Comment 28

[Boris Zbarsky [:bzbarsky]]

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.

Comment 29

[Julien Cristau [:jcristau]]

Not tracking for 52.  Based on comment 28 uplifting may be risky.

Comment 30

[Boris Zbarsky [:bzbarsky]]

Oh, and the patches as written depend on the patches for bug 1324035 a bit.  We'd need to backport part of that too.

Comment 31

[Boris Zbarsky [:bzbarsky]]

Luckily, they don't depend on much of it, because backporting that would be a pain.

Comment 32

[Boris Zbarsky [:bzbarsky]]

Attachment: Prerequisite for making the rest of these patches compile on Aurora. This should land before "Part 1".

Comment 33

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 5 backported to Aurora

The other parts apply just fine

Comment 34

[Boris Zbarsky [:bzbarsky]]

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.

Comment 35

[Julien Cristau [:jcristau]]

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

Comment 36

[Ryan VanderMeulen [:RyanVM]]

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