https://developer.chrome.com/extensions/webNavigation#method-getFrame https://developer.chrome.com/extensions/webNavigation#method-getAllFrames

This patch: - defines the getFrame / getAllFrames API methods - updates the webNavigation.jsom schema - manages getFrame / getAllFrames request/response exchange, request tracking and cleanup when the originating context is closed (which is a part that definitely needs feedback) - WebNavigation is now initialized on the first listener registered OR the first call to getFrame or getAllFrames - prevents WebProgressListener from be uninitialized twice - defines handlers for the getFrame / getAllFrames requests received by the webNavigation framescript (search / collect all the docShell objects , retrieve the frame details that needs to be sent to the main process and routed to the extension context which originated the request)

This patch contains a new test case for the getFrame/getAllFrames API methods.

Comment on attachment 8709084 [details] [diff] [review] 0001-Bug-1190685-webext-Implements-webNavigation.getFrame.patch Review of attachment 8709084 [details] [diff] [review]: ----------------------------------------------------------------- This looks pretty good. I haven't looked over all of WebNavigation.json or WebNavigationContent.js yet, because I think it needs to wait for my current patch to land rather than re-implement cross-process callback logic in yet another way. Sorry to block you on that. I'll get it up for review today. ::: toolkit/components/extensions/schemas/web_navigation.json @@ +21,5 @@ > ], > "functions": [ > { > "name": "getFrame", > + "unsupported": false, It's better to just remove the property. Same for the ones below. @@ +31,5 @@ > "name": "details", > "description": "Information about the frame to retrieve information about.", > "properties": { > "tabId": { "type": "integer", "minimum": 0, "description": "The ID of the tab in which the frame is." }, > + "processId": {"unsupported": false, "type": "integer", "description": "The ID of the process runs the renderer for this tab."}, I think this is still unsupported. ::: toolkit/modules/addons/WebNavigation.jsm @@ +128,5 @@ > + }, > + > + getFrame(tab, context, details, callback) { > + if (!this.initialized) { > + this.init(); I'd rather avoid initializing all of the change listeners when we're just getting details on frames. Let's initialize the frame listeners and the change listeners separately. @@ +145,5 @@ > + > + let reqId = this.addExtensionRequest(context, callback); > + let data = { details, reqId }; > + let mm = tab.linkedBrowser.messageManager; > + mm.sendAsyncMessage("Extension:GetAllFrames:Req", data); I'm trying to avoid adding any more ad-hoc callback routing. We should wait for my current patch to land. It should make this much easier. ::: toolkit/modules/addons/WebNavigationContent.js @@ +110,5 @@ > + * @return {nsIDOMWindow} - the DOMWindow associated to the docShell. > + */ > +function docShellToWindow(docShell) { > + return docShell.QueryInterface(Ci.nsIInterfaceRequestor) > + .getInterface(Ci.nsIDOMWindow); Please align the '.' with the previous line. @@ +135,5 @@ > +function convertWindowToFrameDetail(window) { > + return { > + windowId: getWindowId(window), > + parentWindowId: getParentWindowId(window), > + url: window.location.toString(), |window.location.href|. Also, please try to avoid using |.toString()|. |String(...)| does the job better. @@ +143,5 @@ > +// NOTE: Compute the Error Page loadType value. (See Bug 1190685 for rationale) > +// - LOAD_CMD_NORMAL (as defined in "docshell/base/nsIDocShell.idl"): > +// - LOAD_FLAGS_ERROR_PAGE (as defined in "docshell/base/nsDocShellLoadTypes.h") > +// - MAKE_LOAD_TYPE (as defined in "docshell/base/nsDocShellLoadTypes.h") > +// - LOAD_ERROR_PAGE (as defined in "docshell/base/nsDocShellLoadTypes.h") Why aren't you using the constants directly from the interfaces? @@ +156,5 @@ > + * @param {nsIDocShell} docShell - the docShell object to be converted into a FrameDetail JSON object. > + * @return {FrameDetail} the FrameDetail JSON object which represents the docShell. > + */ > +function convertDocShellToFrameDetail(docShell) { > + if (!docShell instanceof Ci.nsIDocShell) { You need parens, otherwise you're checking |true| or |false| against the interface. Why would this ever not be a docshell? @@ +176,5 @@ > + * @return {FrameDetail} the FrameDetail JSON object which represents the docShell. > + */ > +function findFrame(windowId, docShell) { > + let docShellsEnum = docShell.getDocShellEnumerator( > + Ci.nsIDocShellTreeItem.typeAll, Let's go with |typeContent|. I don't think we really want extensions to know about chrome docshells. @@ +182,5 @@ > + ); > + > + while (docShellsEnum.hasMoreElements()) { > + let docShell = docShellsEnum.getNext(); > + let frameDetail = convertDocShellToFrameDetail(docShell); It would be nice if we didn't have to create a frame detail object for every docshell when we're just trying to match a window ID. How about we just check the window ID before creating a frame detail object for the match? @@ +202,5 @@ > + * @return {nsIDocShell[]} - the array of the frames' docShells collected > + */ > +function getChildDocShells(docShell) { > + let docShellsEnum = docShell.getDocShellEnumerator( > + Ci.nsIDocShellTreeItem.typeAll, |typeContent| @@ +210,5 @@ > + let docShells = []; > + while (docShellsEnum.hasMoreElements()) { > + let docShell = docShellsEnum.getNext(); > + docShell.QueryInterface(Ci.nsIInterfaceRequestor) > + .getInterface(Ci.nsIWebProgress); There's no need to do this. You're just throwing away the webprogress object. @@ +211,5 @@ > + while (docShellsEnum.hasMoreElements()) { > + let docShell = docShellsEnum.getNext(); > + docShell.QueryInterface(Ci.nsIInterfaceRequestor) > + .getInterface(Ci.nsIWebProgress); > + docShells.push(docShell); I'd make this a star function, and yield rather than append to an array. You can use |Array.from(getChildDocShells(), convertDocShellToFrameDetail)| below.

Comment on attachment 8709085 [details] [diff] [review] 0002-Bug-1190685-webext-Add-webNavigation.getFrame-getAll.patch Review of attachment 8709085 [details] [diff] [review]: ----------------------------------------------------------------- It would be nice to have a test that didn't rely on |onErrorOccurred|. ::: browser/components/extensions/test/browser/browser_ext_webNavigation_getFrames.js @@ +9,5 @@ > + background: "(" + function() { > + let results = []; > + results.push(new Promise((resolve) => { > + browser.webNavigation.getAllFrames({ tabId: 0 }, (res) => { > + browser.test.assertEq(res.length, 0, "getAllFrames should return an empty array for non existent tab"); Should it? I'd expect `undefined` with `lastError` set. Why are you assuming `0` is a nonexistent tab? @@ +14,5 @@ > + resolve(); > + }); > + })); > + results.push(new Promise((resolve) => { > + browser.webNavigation.getFrame({ tabId: 0, frameId: 15, processId: 20}, (res) => { Missing space before the } @@ +45,5 @@ > + browser.webNavigation.onErrorOccurred.addListener((errorOccurredDetails) => { > + let { tabId } = errorOccurredDetails; > + browser.webNavigation.getAllFrames({ tabId }, (getAllFramesDetails) => { > + let { frameId } = getAllFramesDetails[0]; > + browser.webNavigation.getFrame({ tabId, frameId, processId: 0 }, (getFrameDetail) => { We don't support |processId| yet. I don't think I'd expect |processId: 0| to do the right thing if we did, though. @@ +73,5 @@ > + } = yield extension.awaitMessage("getAllFrames.details"); > + > + is(errorOccurredDetails.url, NO_CERT_URL, "an onErrorOccurred event should has been raised with the expected URL"); > + > + let errorOccurredFrameFound = getAllFramesDetails.filter((frameDetail) => { |getAllFrameDetails.some(...)| @@ +108,5 @@ > + browser.webNavigation.getAllFrames({ tabId }, (getAllFramesDetails) => { > + let getFramePromises = getAllFramesDetails.map((frameDetail) => { > + let { frameId } = frameDetail; > + return new Promise((resolve) => { > + browser.webNavigation.getFrame({ tabId, frameId, processId: 0 }, resolve); Same thing. We don't support |processId|... @@ +164,5 @@ > + let errorFrames = getAllFramesDetails.filter((el) => el.errorOccurred); > + is(errorFrames.length, 1, "expected number of frames with errors found"); > + is(errorFrames[0].url, NO_CERT_URL, "expected url found with errors"); > + > + is(JSON.stringify(getFrameResults), JSON.stringify(getAllFramesDetails), |JSON.stringify| isn't guaranteed to stringify things in a consistent way. Please use |Assert.deepEqual| instead.

(In reply to Kris Maglione [:kmag] from comment #3) > Comment on attachment 8709084 [details] [diff] [review] > 0001-Bug-1190685-webext-Implements-webNavigation.getFrame.patch > > Review of attachment 8709084 [details] [diff] [review]: > ----------------------------------------------------------------- > > This looks pretty good. > > I haven't looked over all of WebNavigation.json or WebNavigationContent.js > yet, because I think it needs to wait for my current patch to land rather > than re-implement cross-process callback logic in yet another way. Sorry to > block you on that. I'll get it up for review today. Absolutely, The "cross-process callback logic" part is the main reason I asked a preliminary feedback instead of a full review : this kind of "cross-process RPC" is already necessary elsewhere, so it is definitely better if we write it once and reuse it elsewhere in our WebExtensions internals (and in the mean time I wanted to just be sure that I could make the "frames details" data to flow from the content to the main process, like I was planning to) > @@ +31,5 @@ > > "name": "details", > > "description": "Information about the frame to retrieve information about.", > > "properties": { > > "tabId": { "type": "integer", "minimum": 0, "description": "The ID of the tab in which the frame is." }, > > + "processId": {"unsupported": false, "type": "integer", "description": "The ID of the process runs the renderer for this tab."}, > > I think this is still unsupported. Sorry, I forgot to mention in the previous comments the same notes that I put in the weekly meeting notes and in the webNavigation roadmap: By marking this as "unsupported" we're going to create an annoying behaviour for any cross-browsers add-ons which wants to use the getFrame method: - on Chromium browsers, |processId| is a mandatory field of the |getFrame| API method, if it is missing it will raise an exception - On Firefox, |processId| is marked unsupported and if it exists it will raise an exception Initially, I proposed to just ignore it or (even fake it, like suggested by Giorgio), but then I found that the Addon-SDK retrieve a real processID from the runtime service [1], so maybe it could be even better if we can use a proper processId, e.g.: let runtime = Cc["@mozilla.org/xre/app-info;1"].getService(Ci.nsIXULRuntime); let processId = runtime.processID; > ::: toolkit/modules/addons/WebNavigation.jsm > @@ +128,5 @@ > > + }, > > + > > + getFrame(tab, context, details, callback) { > > + if (!this.initialized) { > > + this.init(); > > I'd rather avoid initializing all of the change listeners when we're just > getting details on frames. Let's initialize the frame listeners and the > change listeners separately. I like this idea, in the mean time I'm going to look into it. > @@ +143,5 @@ > > +// NOTE: Compute the Error Page loadType value. (See Bug 1190685 for rationale) > > +// - LOAD_CMD_NORMAL (as defined in "docshell/base/nsIDocShell.idl"): > > +// - LOAD_FLAGS_ERROR_PAGE (as defined in "docshell/base/nsDocShellLoadTypes.h") > > +// - MAKE_LOAD_TYPE (as defined in "docshell/base/nsDocShellLoadTypes.h") > > +// - LOAD_ERROR_PAGE (as defined in "docshell/base/nsDocShellLoadTypes.h") > > Why aren't you using the constants directly from the interfaces? It seems that only LOAD_CMD_NORMAL can be currently retrieved from "Ci.nsIDocShell" (and I'm going to fix it immediately), but the other constants seems to be currently defined and used only in the C++ code. [1]: https://github.com/mozilla/addon-sdk/blob/5ac44b7e088c986bbbd8aaea57b95738eb071ead/lib/sdk/system/runtime.js#L18

(In reply to Luca Greco [:rpl] from comment #5) > Initially, I proposed to just ignore it or (even fake it, like suggested by > Giorgio), but then I found that the Addon-SDK retrieve a real processID from > the runtime service [1], so maybe it could be even better if we can use a > proper processId, e.g.: > > let runtime = > Cc["@mozilla.org/xre/app-info;1"].getService(Ci.nsIXULRuntime); > let processId = runtime.processID; It's probably not ideal to expose OS process IDs to WebExtensions, but I suppose it's OK for now. Ideally, we should probably map them to a separate, internal ID. But that could get complicated. You can get this more directly from `Services.appinfo.processID`, incidentally. > It seems that only LOAD_CMD_NORMAL can be currently retrieved from > "Ci.nsIDocShell" > (and I'm going to fix it immediately), but the other constants seems to be > currently defined and used only in the C++ code. Hm. We shouldn't be using private constants that way. If you need to use it, you should add it to an IDL, which would require review by a peer of that module. I'm not sure this is the right approach, though. I think the `errorOccurred` flag should be set in a lot of cases where we're not showing an error page. We should probably just have a progress listener that stores a flag for the window in a WeakMap whenever a request starts or stops. It should probably use the same logic as the onErrorOccurred listener.

You should be OK to work on this now. Let me know if you have any questions about bug 1210583. I'd suggest splitting this into multiple bugs: 1) The basic implementation of getFrame/getAllFrames. 2) Adding support for processId. 3) Adding support for errorOccurred.

Updated version of the patch which introduces the basic implementation of the webNavigation "getFrame / getAllFrames" methods: - RPC mechanisms ported to the new MessageChannel (new message names registered and handled by augmenting the existent ExtensionContent.jsm module) - webNavigation frames helpers and "getFrame / getAllFrames" implementation moved in the new "WebNavigationFrames.jsm" module, imported and used from the "ExtensionContent.jsm" module and the "WebNavigationContent.js" framescript - ext-webNavigation API methods implementation ported to the new Promise based API - fix getFrame and getAllFrames results on non existent tabs (parameter is null and runtime.lastError is undefined) - removed errorOccurred (moved in a followup), - use typeContent instead of typeAll to iterate in the docShell tree - define a generators to iterate over the docShell tree and reuse it to map all frames and searching for a single frame - updates to the API schema: - getFrame / getAllFrames marked as supported - getFrame's processId parameter marked as optional - explicitly mark errorOccurred as unsupported - minor tweaks: - aligned . on the previous line - removed toString usage

Updated version of the test case: - cleanup (e.g. removed errorOccurred dependency) - use Assert.deepEqual

Comment on attachment 8716082 [details] [diff] [review] 0001-Bug-1190685-webext-Implements-webNavigation.getFrame.patch Review of attachment 8716082 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Thanks. ::: toolkit/components/extensions/ExtensionContent.jsm @@ +699,5 @@ > > receiveMessage({ target, messageName, recipient, data }) { > switch (messageName) { > case "Extension:Capture": > + return this.handleExtensionCapture(data); Let's deal with extracting the message data here rather than passing the raw data to the handlers. return this.handleExtensionCapture(data.options); @@ +701,5 @@ > switch (messageName) { > case "Extension:Capture": > + return this.handleExtensionCapture(data); > + case "Extension:Execute": > + return this.handleExtensionExecute(target, recipient, data); return this.handleExtensionExecute(target, recipient.extensionId, data.options); @@ +743,4 @@ > > + handleWebNavigationGetFrame(data) { > + let deferred = PromiseUtils.defer(); > + deferred.resolve(WebNavigationFrames.getFrame(this.global.docShell, data)); No need to create a deferred object. Just `return WebNavigationFrames.getFrame(this.global.docShell, data)` If you need an explicit promise, you can always use `Promise.resolve(...)` ::: toolkit/components/extensions/ext-webNavigation.js @@ +96,5 @@ > + getFrame(details) { > + let tab = TabManager.getTab(details.tabId); > + if (!tab) { > + // On non existent tabs, getFrame resolve to null and no runtime.lastError is set. > + return Promise.resolve(null); Let's return a rejected promise for an invalid tabId. I'm not sure why Chrome doesn't, but it's inconsistent with what we do elsewhere. ::: toolkit/modules/addons/WebNavigationFrames.jsm @@ +71,5 @@ > + Ci.nsIDocShell.ENUMERATE_FORWARDS > + ); > + > + while (docShellsEnum.hasMoreElements()) { > + let docShell = docShellsEnum.getNext(); No need for the variable. `yield docShellsEnum.getNext();` should do fine. ::: toolkit/modules/moz.build @@ +1,1 @@ > + Extra line.

Comment on attachment 8716083 [details] [diff] [review] 0002-Bug-1190685-webext-Add-webNavigation.getFrame-getAll.patch Review of attachment 8716083 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/extensions/test/browser/browser_ext_webNavigation_getFrames.js @@ +9,5 @@ > + results.push(new Promise((resolve) => { > + // There is no "tabId = 0" because the id assigned by TabManager (defined in ext-utils.js) > + // starts from 1. > + browser.webNavigation.getAllFrames({ tabId: 0 }, (res) => { > + browser.test.assertEq(null, res, "getAllFrames should pass a null value for non existent tab"); Let's check for lastError here. And we may as well just use the promise-based variants now rather than creating an explicit promise. @@ +17,5 @@ > + results.push(new Promise((resolve) => { > + // There is no "tabId = 0" because the id assigned by TabManager (defined in ext-utils.js) > + // starts from 1, processId is currently marked as optional and it is ignored. > + browser.webNavigation.getFrame({ tabId: 0, frameId: 15, processId: 20 }, (res) => { > + browser.test.assertEq(null, res, "getFrame should pass a null value for non existent frame "); I don't think this is valid, because we're already bailing out for the `tabId == 0`. We should use a valid tab ID. Also, you have an extra space at the end of your string. (and "nonexistent" is one word). @@ +130,5 @@ > + Assert.deepEqual(getFrameResults, getAllFramesDetails, "getFrame and getAllFrames should return the same results"); > + > + info(`check frame details collected and retrieved with getAllFrames`); > + > + for (let i = 0; i < collectedDetails.length; i++) { `for (let [i, collected] of collectedDetails.entries())` @@ +138,5 @@ > + is(getAllFramesDetail.frameId, collected.frameId, "frameId"); > + is(getAllFramesDetail.parentFrameId, collected.parentFrameId, "parentFrameId"); > + is(getAllFramesDetail.tabId, collected.tabId, "tabId"); > + > + // Bug-XXX: moz-extension url are resolved as jar urls in webNavigation events Hm. Why?

(In reply to Kris Maglione [:kmag] from comment #11) > Comment on attachment 8716083 [details] [diff] [review] > 0002-Bug-1190685-webext-Add-webNavigation.getFrame-getAll.patch > > Review of attachment 8716083 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: > browser/components/extensions/test/browser/ > browser_ext_webNavigation_getFrames.js > @@ +138,5 @@ > > + is(getAllFramesDetail.frameId, collected.frameId, "frameId"); > > + is(getAllFramesDetail.parentFrameId, collected.parentFrameId, "parentFrameId"); > > + is(getAllFramesDetail.tabId, collected.tabId, "tabId"); > > + > > + // Bug-XXX: moz-extension url are resolved as jar urls in webNavigation events > > Hm. Why? Filed Bug 1246125 to better describe the issue and its possible fix.

(In reply to Luca Greco [:rpl] from comment #12) > Filed Bug 1246125 to better describe the issue and its possible fix. Ah. Yeah, I guess that makes sense. Thanks.

Updated version of the main patch with applied tweaks related to the last review comments: - cleaned up ExtensionGlobal.receiveMessage (extract message data before calling the handlers, removed remaining deferred objects used in previous versions of this patch) - returns a rejected promise with a descriptive error messages when the requested tab or frame does not exist - removed the extra line in moz.build - minor tweak on WebNavigationFrame.jsm (directly yield the docShellsEnum.next() return value) Applied the previous r+ flag.

Updated version of the patch with the new test file with the following tweaks applied based on the last review comments: - port the test case to the new Promise-based API - check the Promise rejects on nonexistent tabs and frames, with the expected error message (non existent frame rejection is tested on an existent tab) Applied the previous r+ flag.

Comment on attachment 8717079 [details] [diff] [review] 0002-Bug-1190685-webext-Add-webNavigation.getFrame-getAll.patch Review of attachment 8717079 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/extensions/test/browser/browser_ext_webNavigation_getFrames.js @@ +8,5 @@ > + let results = [ > + // There is no "tabId = 0" because the id assigned by TabManager (defined in ext-utils.js) > + // starts from 1. > + browser.webNavigation.getAllFrames({ tabId: 0 }).then(() => { > + browser.test.assertTrue(false, "getAllFrames Promise should be rejected on error"); You can use `browser.test.fail` for this.

Applied minor tweaks to the test case (using the "browser.test.fail" method to force a failure when needed is definitely better)

Try push results: - https://treeherder.mozilla.org/#/jobs?repo=try&revision=18b831ed48f5&selectedJob=16496095

https://hg.mozilla.org/integration/fx-team/rev/e941c359f792 https://hg.mozilla.org/integration/fx-team/rev/377d0dc81a91

https://hg.mozilla.org/mozilla-central/rev/e941c359f792 https://hg.mozilla.org/mozilla-central/rev/377d0dc81a91

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/WebNavigation/onHistoryStateUpdated https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/WebNavigation/getAllFrames