nsRFPService::RandomizePixels() takes 100% CPU on maps.google.com if compiled by GCC
Categories
(Core :: Privacy: Anti-Tracking, defect)
Tracking
()
People
(Reporter: stransky, Assigned: emilio)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
48 bytes,
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-beta+
diannaS
:
approval-mozilla-release+
|
Details | Review |
downstream bug: https://bugzilla.redhat.com/show_bug.cgi?id=2251202
If Firefox is compiled with CGG nsRFPService::RandomizePixels() takes 100% CPU on maps.google.com and the maps can't be used.
Profile: https://share.firefox.dev/3GhYrYE
I tested latest trunk with gcc-13.2.1-4.fc38.x86_64 / Fedora 38 and the bug is here. Trunk built with clang doesn't show this bug.
Reporter | ||
Comment 1•1 year ago
|
||
The affected code is here: https://searchfox.org/mozilla-central/rev/66d0e3cac02896d0249c6077742411722e6333f0/toolkit/components/resistfingerprinting/nsRFPService.cpp#1281
Jan, any idea what can be wrong here?
Thanks.
Reporter | ||
Updated•1 year ago
|
Comment 2•1 year ago
|
||
If numNoises is 255, then the loop will iloop (i = 255 won't end, and i++ will likely wrap to 0). Note: this is undefined behavior, so compiler/options/opt can affect this. (thanks to penguin42 on Matrix for finding this!)
Comment 3•1 year ago
|
||
I) think the problem here is the loop limit
// Ensure at least 20 random changes may occur.
uint8_t numNoises = std::clamp<uint8_t>(rnd3, 20, 255);
for (uint8_t i = 0; i <= numNoises; i++) {
I debugged with gdb and to me it looks like I have numNoises=255 - but gdb wont actually print it out for me.
Is
for (uint8_t i=0; i<= 255; i++) {
a solid loop?
(Note the <= and the uint8_t) ?
(I'd filed this as https://bugzilla.redhat.com/show_bug.cgi?id=2251620 )
Assignee | ||
Comment 4•1 year ago
|
||
Yeah it seems the loop is just wrong.
Assignee | ||
Comment 5•1 year ago
|
||
If numNoises is 255 the loop never ends because it wraps around to 0.
Updated•1 year ago
|
Just FYI, for me the bug is always reproducible. So that means numNoises is always 255 for me (?).
Seems weird to me as I thought numNoises should be random.
Assignee | ||
Comment 8•1 year ago
|
||
(In reply to Marcel from comment #7)
Just FYI, for me the bug is always reproducible. So that means numNoises is always 255 for me (?).
Well, or that it gets executed enough times such that it statistically always happens at least once, right?
But it might be that gcc just sees that the loop can't terminate if the value is >=255 and thus it assumes that never happens and removes the clamping or what not. Hard to say without looking at the actual machine code.
Comment 9•1 year ago
|
||
Compilers will absolutely do stuff like that. And it varies by compiler and opt level
Comment 10•1 year ago
|
||
Now now, compilers aren't always that mean!
I've prodded at this with gdb a bit, and I can see there are at least a few different values.
Using the Fedora packages, for me it looks like the current count is at the bottom of R11 and the limit as the bottom of R15.
As well as the FF value, I've seen fb,2e,b3,2e,6c,b1
Which leaves the question of wth is g.maps doing reading the image back so much;
I've got a theory - I think it's the little 'Layers' button at the bottom left - I think it's generating a thumbnail to use for that button.
Comment 11•1 year ago
|
||
bugherder |
Comment 12•1 year ago
•
|
||
This bad loop seems to cause issues with the Dark Reader addon as well:
https://github.com/darkreader/darkreader/issues/11834
https://github.com/darkreader/darkreader/issues/11943
Lots of reports from Fedora there, and in the profiles shared (1, 2) we are stuck in nsRFPService::RandomizePixels()
, called from CanvasRenderingContext2D.getImageData()
.
Assignee | ||
Comment 13•1 year ago
|
||
Comment on attachment 9365551 [details]
Bug 1866409 - Fix RFP canvas noise randomization loop. r=timhuang,tjr
Beta/Release Uplift Approval Request
- User impact if declined: comment 0, and probably also other hangs in the wild.
- Is this code covered by automated tests?: Unknown
- Has the fix been verified in Nightly?: Yes
- Needs manual test from QE?: No
- If yes, steps to reproduce:
- List of other uplifts needed: none
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): Trivial fix.
- String changes made/needed: none
- Is Android affected?: Yes
Updated•1 year ago
|
Comment 14•1 year ago
|
||
Comment on attachment 9365551 [details]
Bug 1866409 - Fix RFP canvas noise randomization loop. r=timhuang,tjr
Approved for 121.0b5.
Comment 15•1 year ago
|
||
uplift |
Updated•1 year ago
|
Comment 16•1 year ago
|
||
Comment on attachment 9365551 [details]
Bug 1866409 - Fix RFP canvas noise randomization loop. r=timhuang,tjr
Approved for 120.0.1 dot release
Comment 17•1 year ago
|
||
uplift |
Updated•1 year ago
|
Updated•1 year ago
|
Comment 19•1 year ago
•
|
||
(In reply to Martin Stránský [:stransky] (ni? me) from comment #0)
I tested latest trunk with gcc-13.2.1-4.fc38.x86_64 / Fedora 38 and the bug is here. Trunk built with clang doesn't show this bug.
I looked at what the loop code looks like in Firefox 120 for Windows x64 (so before patch, compiled with clang too):
// edx is rnd3
cmp dl, 21
mov r11d, 20
cmovae r11d, edx
// r11b is now std::clamp<uint8_t>(rnd3, 20, 255)
inc r11b
movzx edx, r11b
cmp dl, 2
mov r11d, 1
cmovae r11d, edx
// r11b is now 1 if std::clamp<uint8_t>(rnd3, 20, 255) was 255
// otherwise, r11b is now std::clamp<uint8_t>(rnd3, 20, 255) + 1
[...]
loop:
[...]
dec r11b
jne loop
When applied to a rnd3
value in 0..254, this code yields a loop with rnd3+1
turns, as expected from reading the C++ source code. When applied to a rnd3
value of 255 however, it yields a loop with only 1 turn, and not an infinite loop like we may expect. I think that clang didn't care about guaranteeing a particular behavior when rnd3
is 255 because this case breaks forward progress (in simple terms, "infinite loops without side effects are undefined behavior"). I believe that the case where rnd3
had a value of 255 was therefore undefined behavior, thus explaining the difference in behavior across different builds.
Description
•