User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 Build ID: 20121010150351 Steps to reproduce: the following page-mod is created: require("page-mod").PageMod({ include: ["*", "file://*"], attachTo: ["existing", "top", "frame"], contentScriptWhen: 'ready', onAttach: <script> }); as part of an add-on Actual results: When the add-on is installed, the onAttach script is not executed for "existing" multi-frames documents. Only top document is correctly handled. For a new tab (or reloading an existing one), we get the expected behavior. Expected results: It is expected that onAttach script is executed for "existing" top document as well as all frames.

Important precision: I have this problem on version 1.11rc1

Erik, Matteo, can one of you look at this a.s.a.p?

looks related to bug 802987, Matteo?

Hmm maybe not, I'll look at this one.

It's unrelated, this is an actual bug. In fact as we can see: https://github.com/mozilla/addon-sdk/blob/master/lib/sdk/page-mod.js#L182-199 We're applying the pagemod only on existing top document, not frames. A possible solution could be replacing: forEach(function (tab) { // Fake a newly created document mod._onContent(getTabContentWindow(tab)); }); With something like: forEach(function (tab) { // Fake a newly created document let window = getTabContentWindow(tab); getFrames(window).concat(window).forEach(function(win){ mod._onContent(win); }); }); Where `getFrame` is a recursive function that returns a flatten array of any frame contained in the `window` given. Unfortunately it seems we didn't have a unit test for that scenario (and we should), so we didn't catch it before. Erik, if you've already start to working on that you can send the pull request and set me as reviewer, otherwise I can fix it.

I get this bug when using the example, after opening a new tab (this might be unrelated): Program terminated successfully. (jetpack-sdk)evold-10775:test-addon evold$ cfx run Using binary at '/Applications/Firefox.app/Contents/MacOS/firefox-bin'. Using profile at '/var/folders/1l/nh4yxxcd435161ympywl_2800000gn/T/tmpHhej3Q.mozrunner'. info: test-addon: onAttach error: test-addon: An exception occurred. Traceback (most recent call last): File "resource://jid1-ig5zsukau75snq-at-jetpack/addon-sdk/lib/sdk/page-mod.js", line 234, in onReady self._createWorker(window); File "resource://jid1-ig5zsukau75snq-at-jetpack/addon-sdk/lib/sdk/page-mod.js", line 245, in _createWorker this._emit('attach', worker); File "resource://jid1-ig5zsukau75snq-at-jetpack/addon-sdk/lib/sdk/deprecated/events.js", line 123, in _emit return this._emitOnObject.apply(this, args); File "resource://jid1-ig5zsukau75snq-at-jetpack/addon-sdk/lib/sdk/deprecated/events.js", line 153, in _emitOnObject listener.apply(targetObj, params); File "resource://jid1-ig5zsukau75snq-at-jetpack/test-addon/lib/main.js", line 8, in console.log(mod.tab.url); File "resource://jid1-ig5zsukau75snq-at-jetpack/addon-sdk/lib/sdk/content/worker.js", line 517, in return getTabForWindow(this._window); File "resource://jid1-ig5zsukau75snq-at-jetpack/addon-sdk/lib/sdk/tabs/helpers.js", line 15, in getTabForWindow return Tab({ tab: tab }); File "resource://jid1-ig5zsukau75snq-at-jetpack/addon-sdk/lib/sdk/tabs/tab-firefox.js", line 196, in Tab let tab = TabTrait(options); File "resource://jid1-ig5zsukau75snq-at-jetpack/addon-sdk/lib/sdk/deprecated/traits.js", line 114, in Trait return self.constructor.apply(self, arguments) || self._public; File "resource://jid1-ig5zsukau75snq-at-jetpack/addon-sdk/lib/sdk/tabs/tab-firefox.js", line 43, in Tab window.tabs.on(type.name, this._onEvent.bind(this, type.name)); TypeError: window.tabs is undefined Total time: 695.466302 seconds

(In reply to Erik Vold [:erikvold] from comment #6) is bug 804935 now, but I think may be more important than this one.

As I said to Erik before in vydeo, we should probably check the `attachTo` options before iterates all the frames, so a code like that is preferable: forEach(function (tab) { let window = getTabContentWindow(tab); mod._onContent(window); if (mod.attachTo.indexOf("frame") > -1) getFrames(window).forEach(mod._onContent, mod); }); In this way we avoid unnecessary iteration on frames.

Pointer to Github pull-request

Comment on attachment 674840 [details] Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/631 Looks good, please address my comments before landing though.

Commits pushed to master at https://github.com/mozilla/addon-sdk https://github.com/mozilla/addon-sdk/commit/8ef3138638543a2be3a229011d6b5807117d7679 bug 803529 apply page-mods to existing frames https://github.com/mozilla/addon-sdk/commit/f439105bcd9c46704d0ba4944ee10c97ed1cae03 Merge pull request #631 from erikvold/803529-2 Fix Bug 803529 fix for applying page-mods to existing frames r=@gozala

Commit pushed to stabilization at https://github.com/mozilla/addon-sdk https://github.com/mozilla/addon-sdk/commit/176d82da5b7de5c27c8fd50bc9fa7b053eda5412 Merge pull request #631 from erikvold/803529-2 Fix Bug 803529 fix for applying page-mods to existing frames r=@gozala Conflicts: lib/sdk/tabs/utils.js packages/addon-kit/lib/page-mod.js packages/addon-kit/tests/test-page-mod.js packages/api-utils/lib/window/utils.js

Pointer to Github pull-request

Please, forget the last pull, not sure why it was done. Erik, I saw your pull but I didn't see the things we were discussing (also, I asked to be reviewer of this one :) ): - It seems you don't check for the `attachTo` options and iterate all the frames always, that should be definitely fixed unless there is a specific reason for that. - You added the bug for this specific case, but all the other wrong test for `attachTo` are still there, and they should be fixed as as well, because related to this wrong behavior.

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #14) > Please, forget the last pull, not sure why it was done. > > Erik, I saw your pull but I didn't see the things we were discussing (also, > I asked to be reviewer of this one :) ): > > - It seems you don't check for the `attachTo` options and iterate all the > frames always, that should be definitely fixed unless there is a specific > reason for that. Yeah I forgot about this one, sorry, I'll make a new bug for it now though. I should improve the test I included I realize now too, so I will do these together. > - You added the bug for this specific case, but all the other wrong test for > `attachTo` are still there, and they should be fixed as as well, because > related to this wrong behavior. I made a new bug for this one, bug 805321, which I will do today as well.

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #14) > Please, forget the last pull, not sure why it was done. > > Erik, I saw your pull but I didn't see the things we were discussing (also, > I asked to be reviewer of this one Sorry, I missed that :( and thought it'd be good to get Irakli to review the getFrames function.

(In reply to Erik Vold [:erikvold] from comment #16) > Sorry, I missed that :( No problem at all! :) > and thought it'd be good to get Irakli to review the getFrames function. I think I actually used the same approach in the re-implementation of selection module. ;) BTW, I think the check for `frame` in `attachTo` options before calling `getFrames` belongs to this bug, not the bug 805321 (that is specifically for tests). It would be better if you just makes a new pull request with that change on this bug rather than on the "test" bug.

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #17)] > BTW, I think the check for `frame` in `attachTo` options before calling > `getFrames` belongs to this bug, not the bug 805321 (that is specifically > for tests). It would be better if you just makes a new pull request with > that change on this bug rather than on the "test" bug. whoops I meant this is bug 805858 The code wasn't checking that "frames" flag was used before, and would call `mod._onContent` for top level windows even when the "frame" flag was used without "top" flag for `attachTo`.. so I'm not sure it needs to be part of this bug. It seems like a separate performance issue to me. Anyhow I mentioned the bug to Dave, and he doesn't want to hold up the train for 1.11 so it's probably better of as another bug at this point.

Commit pushed to master at https://github.com/mozilla/addon-sdk https://github.com/mozilla/addon-sdk/commit/80e140bb88efbe22c014bc399503ca7f65c6d79d test improvments for bug 803529