1620921 - view image on canvas in new tab / window no longer works

Status: RESOLVED FIXED

(Firefox :: Menus, defect, P1)

Description

[lilydjwg]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:75.0) Gecko/20100101 Firefox/75.0

Steps to reproduce:

• find a canvas image, e.g. here https://tcdw.github.io/servalclock/
• right click on the image
• middle click "View Image" menu entry; or click+left click, shift+left click

Actual results:

Firefox says the URL is invalid.

Expected results:

Firefox should display the image in the newly opened tab / window.

150:21.81 INFO: Last good revision: 398b5e6b2dab72d5be60bc81b0fda2d2c27bfb25
150:21.81 INFO: First bad revision: b9740bd7bf8f1a0656c09a88fe6d7ee00586ee60
150:21.81 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=398b5e6b2dab72d5be60bc81b0fda2d2c27bfb25&tochange=b9740bd7bf8f1a0656c09a88fe6d7ee00586ee60

Being able to view images in new tabs is useful to view images closely or simultaneously without the need to save images on disk. The "Copy Image" is not available on canvas so it's useful for copying images too.

Comment 1

[Alice0775 White]

I can reproduce on Nightly75.0a1 Windows10.

Comment 2

[Jens Stutte [:jstutte]]

Hmm, it seems that this the desired behavior wanted by bug 1472158. I assume, the new tab on the middle-click generates a different process and then the blobURL is not broadcasted any more. The question could be: should the new tab really run in a new process in this case?

Comment 3

[Andrea Marchesini [:baku]]

We are working on an important refactoring of how blobURLs are loaded. Subhamoy is working on it. But in the meantime, we should fix this issue without reverting bug 1472158.

Nika do you know if there is a way to load the new tab on the same process, based on the STR in the description of this bug?

Comment 4

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

There's the sameProcessAsFrameloader mechanism, but it's not Fission-compatible.

The front-end could potentially work around this by not using Blob's at all. I think we could probably just have the new tab link to the image URL (unless there's a reason we're not doing that in the first place). I that's the preferred method, this should probably go under Firefox :: Menus.

Comment 5

[Jens Stutte [:jstutte]]

Tentatively assigned to Firefox::Menus then.

Comment 6

[:Gijs (he/him)]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #4)

The front-end could potentially work around this by not using Blob's at all. I think we could probably just have the new tab link to the image URL (unless there's a reason we're not doing that in the first place). I that's the preferred method, this should probably go under Firefox :: Menus.

The bug is about HTML canvas, where there is no URL. blob: is used to get a URL to refer to the (saved in memory) image that we create from the canvas. Perhaps we could use sameProcessAsFrameloader for now, and when the refactor from comment #3 arrives we could switch to whatever the new mechanism is, or look at how to update sameProcessAsFrameloader so it is fission-compatible (sameProcessAsBrowsingContext, perhaps?). It might not be an issue for fission anyway; the image will be in tab with origin X, the blob URL gets created with the same origin X, and so the new process selected should also be the (same) process for origin X.

Mike, does that sound OK?

Comment 7

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

(In reply to :Gijs (he/him) from comment #6)

The bug is about HTML canvas,

D'oh, missed that part. Thanks.

Perhaps we could use sameProcessAsFrameloader for now, and when the refactor from comment #3 arrives we could switch to whatever the new mechanism is...
Mike, does that sound OK?

Yes it does!

Comment 8

[:Gijs (he/him)]

Ever since bug 346849 I feel a weird kind of affinity for these menuitems, so I guess I might as well own fixing it.

Comment 9

[:Gijs (he/him)]

(oh, but wontfix for 74 because it's releasing tomorrow and we're all out of beta runway)

Comment 10

[Jens Stutte [:jstutte]]

Hi Gijs, can we still expect a patch for 75 then?

Comment 11

[:Gijs (he/him)]

(In reply to Jens Stutte [:jstutte] from comment #10)

Hi Gijs, can we still expect a patch for 75 then?

Sorry, I've been working on some higher-prio stuff. I got stuck on the new window case, where forcing the use of the "right" process is non-trivial, but I just talked with baku and there's an easier solution here. Patch coming up.

Comment 12

[:Gijs (he/him)]

Attachment: Bug 1620921 - restart broadcasting system principal blob URIs to ensure 'view image' works correctly, r?baku

Comment 13

[:Gijs (he/him)]

Adding a test to ensure this doesn't break again...

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0f0839c074a2
restart broadcasting system principal blob URIs to ensure 'view image' works correctly, r=baku

Comment 15

[Stefan Hindli [:stefan_hindli]]

Backed out changeset 0f0839c074a2 (Bug 1620921) for windows 2012 build bustage CLOSED TREE

https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&selectedJob=293732582

https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=293732582&repo=autoland&lineNumber=2662

Comment 16

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9d40eab931f6
Backed out changeset 0f0839c074a2 for windows 2012 build bustage CLOSED TREE

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d7b1a9284842
restart broadcasting system principal blob URIs to ensure 'view image' works correctly, r=baku CLOSED TREE

Comment 18

[:Gijs (he/him)]

This relanded, thanks Stefan!

Comment 19

[Andreea Pavel [:apavel]]

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

Comment 20

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:Gijs, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 21

[:Gijs (he/him)]

Last beta tomorrow, rc next week, so a bit late in the cycle... this functionality seems kind of edgecase (only breaks for canvas images, not all images) and blob URI broadcasting might have other side-effects, so let's have it ride the trains.