1339440 - documentUrlPatterns in the contextMenus does not work on frames properly

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P2)

Description

[Johny G.]

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:54.0) Gecko/20100101 Firefox/54.0
Build ID: 20170213030206

Steps to reproduce:

documentUrlPatterns in the contextMenus does not work on frames properly

This bug applies to WebExtensions contextMenus API. 
Specification states that context "frame" use documentUrlPatterns to filter its src attribute,
in distinction of "image", "link" and "video" that use targetUrlPatterns.

1.
To reproduce issue, create simple extension compound of two files:
manifest.json:
{
"manifest_version": 2,
"version": "1.0",
"name": "frame context menu",
"permissions": ["contextMenus"],
"background": {"scripts": ["sample_menu.js"]}
}

sample_menu.js:
chrome.contextMenus.create({
	"title": "FRAME context menu",
	"contexts": ["frame"],
	"documentUrlPatterns": ["<all_urls>"]
});
function playOnClick(info, tab) {
    console.log(JSON.stringify(info));    
}
chrome.contextMenus.onClicked.addListener(playOnClick);

2.
Install extension at 'about:debugging' and click debug to show javascript console.
In new tab navigate to https://developers.google.com/youtube/youtube_player_demo
This is sample website with embedded youtube player that uses iframe.

Right click on player (right click twice, as first time player menu appears).
Our context menu shows, hover and click on it. 

Javascript console displays:
{"menuItemId":1, "editable":false, "parentMenuItemId":0, "linkUrl":"", "srcUrl":"",
"pageUrl":"https://developers.google.com/youtube/youtube_player_demo",
"frameUrl":"https://www.youtube.com/embed/M7lc1UVf-VE?...cut.as.not.important", "modifiers":[]}

Value in frameUrl confirms, that iframe src attribute is filled with url,
that matches pattern "https://www.youtube.com/embed/*"

3.
Edit sample_menu.js and replace <all_urls> in documentUrlPatterns to look like: 

"documentUrlPatterns": ["https://www.youtube.com/embed/*"]

Reload extension and once again on the same webpage right click on youtube player.
Our context menu does not show, but should. In Chrome this works and menu shows.

4.
Once again edit sample_menu.js and add to documentUrlPatterns "https://developers.google.com/*"
it will look like:
"documentUrlPatterns": ["https://developers.google.com/*", "https://www.youtube.com/embed/*"]

Reload extension and once again on the same webpage right click on youtube player.
Menu shows. This is a clue what might be wrong. Probably implementation first checks if documentUrlPatterns matches parent 
url, and if not, do not render menu even if there is "frame" among contexts attribute in the menu.

This was tested on 64bit Windows 7 and Firefox 51.0.1 64bit, and nightly build 54.0a1 (2017-02-13) 64bit.
I don't have any other extensions installed.

Comment 1

[:shell escalante]

shane and kris were going to talk about this one

Comment 2

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1339440 use frameUrl to match context menus on frames

Review commit: https://reviewboard.mozilla.org/r/118380/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/118380/

Comment 3

[Shane Caraveo (:mixedpuppy)]

The documentation is vague on what is expected here: 

documentUrlPatternsOptional
    array of string. Lets you restrict the item to apply only to documents whose URL matches one of the given match patterns. This applies to frames as well.

It does however make sense that the match is against the frameUrl, not the url of the parent.

At a minimum, we need to add a test form "frame" since one did not exist.

Comment 4

[Kris Maglione [:kmag]]

Comment on attachment 8845191 [details]
Bug 1339440 use frameUrl to match context menus on frames

https://reviewboard.mozilla.org/r/118380/#review121786

Still not really sold on this behavior, but fine.

::: browser/components/extensions/ext-contextMenus.js:520
(Diff revision 1)
> -    let pageURI = Services.io.newURI(contextData.pageUrl);
> +    let pageURI = contextData.inFrame ? Services.io.newURI(contextData.frameUrl)
> +                                      : Services.io.newURI(contextData.pageUrl);

Please move the ternary inside the `newURI` call, or ideally something like `contextData[inFrame ? "frameUrl" : "pageUrl"]`

Comment 5

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8845191 [details]
Bug 1339440 use frameUrl to match context menus on frames

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118380/diff/1-2/

Comment 6

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s d78792d5e9a8 -d e2d9ac61dd7d: rebasing 382228:d78792d5e9a8 "Bug 1339440 use frameUrl to match context menus on frames r=kmag" (tip)
merging browser/components/extensions/test/browser/browser-common.ini
merging browser/components/extensions/test/browser/head.js
warning: conflicts while merging browser/components/extensions/test/browser/head.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 7

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8845191 [details]
Bug 1339440 use frameUrl to match context menus on frames

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118380/diff/2-3/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/075395ba45fe
use frameUrl to match context menus on frames r=kmag

Comment 9

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/autoland/rev/2b8f086b702f for https://treeherder.mozilla.org/logviewer.html#?job_id=84153815&repo=autoland

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Driving by... looking at https://hg.mozilla.org/integration/autoland/rev/075395ba45fe#l1.13,

I'm pretty sure:
> let pageURI = Services.io.newURI(contextData["inFrame" ? "frameUrl" : "pageUrl"]);
should have been:
> let pageURI = Services.io.newURI(contextData[inFrame ? "frameUrl" : "pageUrl"]);

The actual code that landed should always return "frameUrl" since the string "inFrame" is always truthy. I don't see where the inFrame variable comes from though, but I'll assume it's present already since comment 4 mentions it.

Comment 11

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8845191 [details]
Bug 1339440 use frameUrl to match context menus on frames

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118380/diff/3-4/

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9aa2aa7a42d2
use frameUrl to match context menus on frames r=kmag

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/9aa2aa7a42d2