1237025 - View Image Info context menu option uses unsafe cpows

Status: RESOLVED FIXED

(Firefox :: Page Info Window, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

STR:

1) With an e10s enabled browser, visit the attachment
2 [review]) Open the Browser Console
3) Right click on the item and choose "View Image Info"

ER:

No "unsafe cpow usage" warnings in the console.

AR:

Several "unsafe cpow usage" warnings in the console.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Hey gw280, just a heads up that my patch in bug 1238180 should take care of the first unsafe CPOW that gets used in nsContextMenu.js. There are still a few that'll be used for View Image Info from inside pageInfo.js, however.

Comment 2

[George Wright (:gw280) (needinfo me!)]

Attachment: 0001-Bug-1237025-Stop-using-CPOWs-for-View-Image-Info-con.patch

Comment 3

[George Wright (:gw280) (needinfo me!)]

I think we can just pass null in nsContextMenu.js; I couldn't see any reason why it makes any difference for us to pass in this.target.ownerDocument.defaultView.top.document or not.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8709732 [details] [diff] [review]
0001-Bug-1237025-Stop-using-CPOWs-for-View-Image-Info-con.patch

Review of attachment 8709732 [details] [diff] [review]:
-----------------------------------------------------------------

This is the right idea, but I think there's an edge case where this will not work. See below - I have a suggestion that I think will simplify this. Thanks gw280!

::: browser/base/content/pageinfo/pageInfo.js
@@ +504,5 @@
>    radioGroup.focus();
> +
> +  getImageInfo.then(function(data) {
> +    if (data) {
> +      gImageElement = {currentSrc: data.currentSrc,

This could, theoretically, result in us getting gImageElement too late. gImageElement is used here:

https://dxr.mozilla.org/mozilla-central/rev/6764bc656c1d146962d53710d734c2ac87c2306f/browser/base/content/pageinfo/pageInfo.js#632

Which is run when we add an image row to the Media tab - the adding of which is done asynchronously once loadPageInfo is called (and the information is only used so that we select the right image in the list). For a small list of images (say a single image), we could end up adding the image row to the Media tab before getImageInfo resolves, which would result in us not selecting the image as intended.

What I'd suggest instead is that we send down the imageElement CPOW with the PageInfo:getData message that goes down here:

https://dxr.mozilla.org/mozilla-central/rev/6764bc656c1d146962d53710d734c2ac87c2306f/browser/base/content/pageinfo/pageInfo.js#363

Then send up the image data to the parent again here: https://dxr.mozilla.org/mozilla-central/rev/6764bc656c1d146962d53710d734c2ac87c2306f/browser/base/content/content.js#1053

Which is guaranteed to be received before the media stuff starts coming up from here: https://dxr.mozilla.org/mozilla-central/rev/6764bc656c1d146962d53710d734c2ac87c2306f/browser/base/content/content.js#1056

Comment 5

[Ryan VanderMeulen [:RyanVM]]

FYI, this is going to need a rebased patch for Aurora.

Comment 6

[George Wright (:gw280) (needinfo me!)]

Yep, figured as much. Thanks

Comment 7

[George Wright (:gw280) (needinfo me!)]

Attachment: 0001-Bug-1237025-Stop-using-CPOWs-for-View-Image-Info-con.patch

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8711176 [details] [diff] [review]
0001-Bug-1237025-Stop-using-CPOWs-for-View-Image-Info-con.patch

Review of attachment 8711176 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks gw280! Note that I didn't test this locally, so I assume you've manually tested to ensure that there are no more unsafe CPOW usage warnings here.

Please address / rebut my nits, and then land away.

::: browser/base/content/content.js
@@ +936,5 @@
>  
> +  getImageInfo: function(imageElement) {
> +    let imageInfo = null;
> +    if (imageElement) {
> +      imageInfo = {currentSrc: imageElement.currentSrc,

Style nit - please format like this:

imageInfo = {
  currentSrc: imageElement.currentSrc,
  width: imageElement.width,
  height: imageElement.height,
  imageText: imageElement.title || imageElement.alt,
};

::: browser/base/content/pageinfo/pageInfo.js
@@ +377,5 @@
>      gDocInfo = docInfo;
>  
> +    let imageInfo = pageInfoData.imageInfo;
> +    if (imageInfo) {
> +      gImageElement = {currentSrc: imageInfo.currentSrc,

If we're not ever going to do anything with imageInfo outside this block, might as well just set gImageElement to imageInfo here.

In fact, since imageInfo is null in the case where we passed an invalid image element, this whole block can probably be simplified to:

gImageElement = pageInfoData.imageInfo;

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fecacfd2f995

Comment 10

[George Wright (:gw280) (needinfo me!)]

Attachment: 0001-Bug-1237025-Stop-using-CPOWs-for-View-Image-Info-con.patch

Final patch which landed. Carrying r+, and requesting aurora uplift.

Approval Request Comment
[Feature/regressing bug #]: not a regression
[User impact if declined]: unsafe CPOWs when using the context menu's view image info option, so potential for jank or deadlock
[Describe test coverage new/current, TreeHerder]: m-c
[Risks and why]: low risk, just some small changes to remove CPOW usage by sending async messages across to the content process to gather relevant image data for us.
[String/UUID change made/needed]: none

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/fecacfd2f995

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8711184 [details] [diff] [review]
0001-Bug-1237025-Stop-using-CPOWs-for-View-Image-Info-con.patch

Looks good on Try.

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8711184 [details] [diff] [review]
0001-Bug-1237025-Stop-using-CPOWs-for-View-Image-Info-con.patch

Improve the stability, taking it.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4624d3e7ba1a

Comment 15

[Mayur Patil]

[bugday-20160323]

Status: RESOLVED,FIXED -> UNVERIFIED

Comments:
STR: Not clear.
Developer specific testing

Component: 
Name			Firefox
Version			46.0b9
Build ID		20160322075646
Update Channel          beta
User Agent		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS			Windows 7 SP1 x86_64

Expected Results: 
Developer specific testing

Actual Results: 
As expected