885289 - Improve context menu behaviour for srcdoc iframes

Status: RESOLVED FIXED

(Firefox :: Menus, defect)

Description

[James Kitchener (:jkitch)]

Bug 802895 disables the "This Frame" context menu item for srcdoc iframes.  This was done to prevent "show only this frame" etc. displaying arbitrary web content in the about protocol that was previously reserved for browser-internal/addon pages

A consequence of this is that it will hide the "view image"/"view video" context menu items for images/videos in srcdoc frames, which probably isn't optimal.

To fix that we'd probably need to track inFrame and inSrcdocFrame separately, so that srcdoc iframes retain a "this frame" sub-menu, but without the offending menu options.

Comment 1

[James Kitchener (:jkitch)]

Attachment: menu

This hides the frame entries that are undesirable or do not make sense for srcdoc iframes.

The question is, whether suppressing the ability to open srcdoc iframes in isolation is still undesirable.

Since the decision to suppress them was made, new identity elements have been introduced to indicate secure Firefox-internal pages.  If the srcdoc document in isolation displayed the standard grey globe icon (with an about:srcdoc URI), would it be clear that it is untrusted, arbitrary content?

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 778431 [details] [diff] [review]
menu

Someone else (bz?) should review the nsContentUtils change. At first glance it seems odd to be special-casing those particular schemes (perhaps it should be a check for nested URIs in general, using the innermost if present?).

I'll delegate the nsContextMenu reviews to mconley.

Comment 3

[James Kitchener (:jkitch)]

Comment on attachment 778431 [details] [diff] [review]
menu

Requesting review for the nsContentUtils portion of the patch.

The check is necessary to allow viewing the source of a srcdoc document to work in debug builds.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 778431 [details] [diff] [review]
menu

r=me on the contentutils change.

Comment 5

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

Comment on attachment 778431 [details] [diff] [review]
menu

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

r=me with my concerns resolved. Thanks!

::: browser/base/content/test/test_contextmenu.html
@@ +1060,5 @@
>      select_inputtext = subwindow.document.getElementById("test-select-input-text");
>      select_inputtext_password = subwindow.document.getElementById("test-select-input-text-type-password");
>      plugin = subwindow.document.getElementById("test-plugin");
>      longdesc = subwindow.document.getElementById("test-longdesc");
> +    srcdoc = subwindow.document.getElementById("test-srcdoc");

Unless I'm mistaken, srcdoc should be added to the list of variables here: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/test_contextmenu.html?force=1#983

::: content/base/src/nsContentUtils.cpp
@@ +6105,5 @@
>      // srcdoc loads, but the URI is non-resolvable in cases where it is not).
>      if (aForceOwner) {
>        nsAutoCString uriStr;
>        aURI->GetSpec(uriStr);
> +      if(!uriStr.EqualsLiteral("about:srcdoc") && 

Trailing whitespace.

Comment 6

[James Kitchener (:jkitch)]

Attachment: patch

Changes applied

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 779686 [details] [diff] [review]
patch

You can just use the checkin-needed bug keyword in the future :)

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Backed out for mochitest-bc orange.
https://hg.mozilla.org/integration/mozilla-inbound/rev/5005e426f324

https://tbpl.mozilla.org/php/getParsedLog.php?id=25718230&tree=Mozilla-Inbound

Comment 10

[Ed Morley [:emorley]]

False alarm, these were due to bug 890690 which has now been backed out.

This can reland - sorry for the churn!

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1514cf837dbe

Sorry again for the goof :(

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1514cf837dbe

Comment 13

[Jean-Yves Perrier [:teoli]]

Doesn't seem to have an impact for developers, no doc needed. Reset the keyword with an explanation if you disagree.