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

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Julien Wajsberg [:julienw]]

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.

I can reproduce this quite easily, for example on https://lemonde.fr.

Comment 1

[Julien Wajsberg [:julienw]]

Here is such a profile: https://share.firefox.dev/3Lr3NSU

Comment 2

[Timothy Nikkel (:tnikkel)]

How do you know that they are incorrect? What in the profile do I look at to know that an Image Load marker has the wrong innerwindowid?

Comment 3

[Julien Wajsberg [:julienw]]

See also this profile https://share.firefox.dev/369IpBz
Captured with a load of cnn.com while running rr, so here is a pernosco session: https://pernos.co/debug/8NVQl2cjOZD7X7acXJk6sA/index.html
3 markers Image Load have innerWindowID values of 1, 23, 25, that are not part of the array profile.pages.

Comment 4

[Julien Wajsberg [:julienw]]

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

How do you know that they are incorrect? What in the profile do I look at to know that an Image Load marker has the wrong innerwindowid?

They're not part of the profile.pages array, but it may also be that that array doesn't have all entries it should have.

Comment 5

[Timothy Nikkel (:tnikkel)]

This is a data race on imgRequest::mInnerWindowId.

https://searchfox.org/mozilla-central/rev/89015dc2d9df7d1c15a7eea31859d28df6460ba9/image/imgRequest.h#267

Before bug 1580820 mInnerWindowId was constant, but that bug added a call that updates mInnerWindowId after Init.

Randall, is it expected that your data race analysis work wouldn't have found this?

Comment 6

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1755032. Fix data race on imgRequest::mInnerWindowId. r?aosmond

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

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

This is a data race on imgRequest::mInnerWindowId.

https://searchfox.org/mozilla-central/rev/89015dc2d9df7d1c15a7eea31859d28df6460ba9/image/imgRequest.h#267

Before bug 1580820 mInnerWindowId was constant, but that bug added a call that updates mInnerWindowId after Init.

Randall, is it expected that your data race analysis work wouldn't have found this?

It would only be found if the author of the patch had realized that that now should be protected, and had marked it (or in the case of when this patch was done, moved it into the "protected by mMutex" block).

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/50a3c4c75a51
Fix data race on imgRequest::mInnerWindowId. r=aosmond

Comment 9

[BugBot [:suhaib / :marco]]

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

Comment 10

[Julien Wajsberg [:julienw]]

Thanks for the quick fix!

Comment 11

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/50a3c4c75a51

Comment 12

[Timothy Nikkel (:tnikkel)]

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

Thanks for the quick fix!

It'd be great if you could verify the original issue is fixed for you.

Comment 13

[Timothy Nikkel (:tnikkel)]

Comment on attachment 9263637 [details]
Bug 1755032. Fix data race on imgRequest::mInnerWindowId. r?aosmond

Beta/Release Uplift Approval Request

• User impact if declined: profiler page ids could be wrong, images might hit the network when we should just use the cache. both of these aren't common, but we should fix them.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• 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): just using an existing lock when accessing a field on different threads
• String changes made/needed:

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: fix a data race.
• User impact if declined: profiler page ids could be wrong, images might hit the network when we should just use the cache. both of these aren't common, but we should fix them.
• Fix Landed on Version: 99
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): just using an existing lock when accessing a field on different threads

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9263637 [details]
Bug 1755032. Fix data race on imgRequest::mInnerWindowId. r?aosmond

This seems edge case enough that it can ride the trains.

Comment 15

[Pascal Chevrel (relman team) -> :pascalc]

Comment on attachment 9263637 [details]
Bug 1755032. Fix data race on imgRequest::mInnerWindowId. r?aosmond

This is an old regression with no duplicates, I think it can ship with 99.

Comment 16

[Timothy Nikkel (:tnikkel)]

I was requesting uplift based on the fact that it's a data race, not based on the effect on users if it makes a difference.

Comment 17

[Julien Wajsberg [:julienw]]

It looks like it's not fully fixed Timothy, sorry about that :/ I'll try to capture another pernosco session tomorrow. Should we reopen this bug, or file a new one?

Comment 18

[Timothy Nikkel (:tnikkel)]

A new bug would be good.