1756551 - [profiler markers] "Image Load" markers for data URLs seem to have incorrect innerWindowID values

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect, P2)

Description

[Julien Wajsberg [:julienw]]

+++ This bug was initially created as a clone of Bug #1755032 +++

From a profile, this is the command I use in the console:

ids = [...new Set(filteredMarkers.map(marker => marker.data && marker.data.innerWindowID))].filter(id => id && profile.pages.every(page => page.innerWindowID !== id));
filteredMarkers.filter(marker => marker.data && ids.includes(marker.data.innerWindowID));

On Content Processes, with this command line, I get Image Load markers for data urls.

The STR I use is roughly:
0. Start the profiler.

1. Open cnn.com in a tab;
2. In another tab, open https://lemonde.fr.
3. Capture

The faulty markers are in the thread for lemonde.fr.

Unfortunately firefox crashes with rr and the profiler today, so it's difficult to capture :/ Happy to instrument my local copy with printf or gather some logs if that can help.

Comment 1

[BugBot [:suhaib / :marco]]

Set release status flags based on info from the regressing bug 1580820

Comment 2

[Julien Wajsberg [:julienw]]

I managed to record a pernosco session after tweaking a recent patch from Florian. Here is the pernosco link: https://pernos.co/debug/bmOx7MB_8uriHZbuxrZ_IQ/index.html

Here is the profile I got: https://share.firefox.dev/3sYpYHJ, with Image Load markers that have their innerWindowID = 1 that's not present in profile.pages.

On a side note, it might be a good idea to not record the full string if it's a data url, I'm afraid of malicious webpages DOSing the profiler...

Comment 3

[Timothy Nikkel (:tnikkel)]

(In reply to Julien Wajsberg [:julienw] from comment #2)

On a side note, it might be a good idea to not record the full string if it's a data url, I'm afraid of malicious webpages DOSing the profiler...

Does it not already do that? https://searchfox.org/mozilla-central/rev/ad38c9d1f0a9036c4da5271a849f47342b68adc8/image/ImageFactory.cpp#117

Comment 4

[Timothy Nikkel (:tnikkel)]

We're passing a uint64_t to a function that takes a uint32_t.

https://searchfox.org/mozilla-central/rev/ad38c9d1f0a9036c4da5271a849f47342b68adc8/image/imgRequest.cpp#834

Bug 1139225 added this.

Comment 5

[Timothy Nikkel (:tnikkel)]

Also found bug 1756587 and bug 1756588 by searching the tree for the regular expression 32.*innerwin

Comment 6

[Timothy Nikkel (:tnikkel)]

Apparently we don't get compiler warnings for this because -Wconversion isn't on https://clang.llvm.org/docs/DiagnosticsReference.html#wconversion

There is also -Wshorten-64-to-32 which might be a flag that is easier to enable and more of the warnings it finds will be real bugs.

Comment 7

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1756551. Parameter aInnerWindowId on PrepareForNewPart in imgRequest.cpp is uint64_t not uint32_t. r?aosmond

Comment 8

[Christian Holler (:decoder)]

(In reply to Timothy Nikkel (:tnikkel) from comment #6)

Apparently we don't get compiler warnings for this because -Wconversion isn't on https://clang.llvm.org/docs/DiagnosticsReference.html#wconversion

There is also -Wshorten-64-to-32 which might be a flag that is easier to enable and more of the warnings it finds will be real bugs.

I've been investigating this, but the amount of warnings even with that flag is very high.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/41da41eb9597
Parameter aInnerWindowId on PrepareForNewPart in imgRequest.cpp is uint64_t not uint32_t. r=aosmond

Comment 10

[Julien Wajsberg [:julienw]]

(In reply to Timothy Nikkel (:tnikkel) from comment #3)

(In reply to Julien Wajsberg [:julienw] from comment #2)

On a side note, it might be a good idea to not record the full string if it's a data url, I'm afraid of malicious webpages DOSing the profiler...

Does it not already do that? https://searchfox.org/mozilla-central/rev/ad38c9d1f0a9036c4da5271a849f47342b68adc8/image/ImageFactory.cpp#117

ah, thanks for double checking!

And also thanks for finding all these issues.

Comment 11

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/41da41eb9597

Comment 12

[Timothy Nikkel (:tnikkel)]

Kelsey also pointed me to this

https://searchfox.org/mozilla-central/rev/04dbb1a865894aec20eb02585aa75acccc0b72d5/dom/canvas/moz.build#214

I tried adding that to just image/ but it generated a ton of spew for something about our AtomicBitfields implementation. I tried adding just -Wshorten-64-to-32 to just image/, that trips of a lot of spew, nsTArray for example.

Comment 13

[Ardelean Oana, Desktop Test Engineering [:oardelean]]

I was unable to reproduce this bug using Profiler on Win10. Would you be so kind as to confirm this fix on latest beta/nightly?
Thank you.

Comment 14

[Julien Wajsberg [:julienw]]

Yes, I'm quite sure I verified it when it landed! Thanks for checking

Comment 15

[Ardelean Oana, Desktop Test Engineering [:oardelean]]

Marked as Verified based on Comment 14.