Closed Bug 804935 Opened 12 years ago Closed 11 years ago

TypeError: window.tabs is undefined

Categories

(Add-on SDK Graveyard :: General, defect, P1)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: evold, Assigned: zer0)

References

Details

Attachments

(1 file, 1 obsolete file)

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
The example was from bug 803529
and I get the error when opening a tab for erikvold.com
(In reply to Erik Vold [:erikvold] from comment #1)
> The example was from bug 803529

	require("page-mod").PageMod({
        include: ["*", "file://*"],
        attachTo: ["existing", "top", "frame"],
        contentScriptWhen: 'ready',
        onAttach: <script>
        });
ok I forgot am important piece:

	require("page-mod").PageMod({
        include: ["*", "file://*"],
        attachTo: ["existing", "top", "frame"],
        contentScriptWhen: 'ready',
        onAttach: function(mod) {
          console.log(mod.tab.url);
        }
        });
I did some test, so: this but is a regression probably given by the tabs refactoring, but it's totally unrelated by `attachTo` bug. Basically it's throwing the exception you mentioned when only we're trying to access to a `tab` property of the worker:
 
    require("page-mod").PageMod({
      include: ["*"],
      onAttach: function(worker) {
        console.log(worker.tab);
      }
    });

But any other worker's functionality is still fine:

    require("page-mod").PageMod({
      include: ["*"],
      contentScript: "self.postMessage('hello')",
      onAttach: function(worker) {
        worker.on("message", function(data) {
          console.log(data);
        })
      }
    });

So, I won't block the `attachTo` bug for this one, but because it's a regression and it's raise an exception I agree this has to be fixed as soon as possible.

If it's fine to you, I can take the `attachTo` meanwhile you're working on that one.
Surely this should cause tests to fail?
Apparently, no. I tried to isolate the problem, so it seems that if you run the same first code BUT you add on top:

    require("tabs");

Everything works as expected.

I have the feeling it could be a regression given by the tabs huge re-work done recently, but maybe not.
I collected also some other data and I'm investigating deeper on it, if I'm not coming with a good solution in the meanwhile, as soon as Erik will be available I will check with him. He should knows tabs module better after his re-implementation.
“... and don't call me Shirley.” (Sorry I couldn't resist)
Okay, actually it seems more that is `require("windows")` to makes the differences ("tabs" calls requires "windows" internally), that in firefox is the module "sdk/windows/firefox".

It seems that without it the `tab.ownerDocument.defaultView` is a `ChromeWindow` instead of a `BrowserWindow`, that's why `window.tabs` is not defined.
It seems that replace this line (https://github.com/mozilla/addon-sdk/blob/master/lib/sdk/tabs/tab-firefox.js#L35):

let window = this.window = options.window || getOwnerWindow(this._tab);

With:

let window = this.window = options.window || BrowserWindow({window: getOwnerWindow(this._tab)});


Did the trick. We basically fall back to a `ChromeWindow` instead of our `BrowserWindow` if nothing is provided.

Apparently if we require "sdk/windows/firefox" we are creating those windows and we pass them when we create `Tab` in the options, that's why we don't fall back to a `ChromeWindow`. 

However, this kind of dependencies is subtle, not so evident.
After talking to Matteo, this bug sounds related to a change to the getTabForWindow function which was moved from some place (will find out shortly) to the `tabs/helpers.js`.  I remember doing the move I think because I wanted to remove a require dependency to a windows module (probably a high level one) because it was causing an issue on Fennec and didn't seem necessary.

Matteo figured out that this caused a default to be used in the Tab constructor that was a ref to a chromeWindow when and instance to a BrowserWindow instance (from the sdk) was needed.
Assignee: nobody → zer0
So the getTabForWindow came from `api-utils/tabs/tab.js` and it depended on a getOwnerWindow function from `api-utils/windows/utils.js`.

Matteo: the `window = options.window || default` was introduced by my Fennec changes in order to remove the dependency.  So this is a regression.  It looks like this default should be wrapped in a BrowserWindow constructor as it was here: https://github.com/mozilla/addon-sdk/blob/stabilization/packages/api-utils/lib/tabs/tab.js#L250

Jeff: There is a test for this function in `test-tabs.js` called `testGetTabForWindow` but there is obviously some problem with it..
* `testGetTabForWindow` is in `test-tab.js`
I guess that the reason because `testGetTabForWindow` is passing, it's because it's compare mainly the `Tab`s instances, not the `window` property. Also, the test requires `tabs` to works, that how we saw bypass this error.

I believe that the all those problems are given by the usage of Trackers in `windows` and `tabs` modules. I fixed this particular issue, but we should seriously thinking about remove Trackers from our code base – luckily, they're all deprecated, so it's just matter of time.
As I mentioned to Erik, I also left the `Tab` constructor of Fennec as is, because even if it uses the same approach, internally it requires a `ChromeWindow` not a `BrowserWindow`. We should definitely makes `Tab` for Fennec and Firefox be more consistent, but this is out of the scope of this bug – and I guess it will be refactored as well as soon as we're going to remove Traits.
Pointer to Github pull-request
Attachment #674838 - Flags: review?(evold)
Hey Matteo, can you take a look at my comment in the pull request for this bug when you have a chance? I'd like to land this to see if it affects bug 810741
Flags: needinfo?(zer0)
Flags: needinfo?(zer0)
Attachment #674838 - Flags: review?(evold) → review+
Attachment #674838 - Flags: review+ → review-
(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #16)
> Created attachment 674838 [details]
> Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/630
> 
> Pointer to Github pull-request

when I run cfx test on this branch I get:

5659 of 5666 tests passed.
That's odd, because the test are only 5663 in this branch; and when I run it I got 5663 on 5663.

Notice that this is related on Firefox Desktop, on this branch the tests are not skipped yet for Firefox Mobile.

Erik, could you double check and tell me more about it? Or maybe clone the branch by scratch and try again? Thanks!
Flags: needinfo?(evold)
Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/08e9d03075db683fb7b5ddc617f1076ac0c93f8d
Bug 804935 - TypeError: window.tabs is undefined

It was a regression during fennec implementation of windows and tabs. A `ChromeWindow` was used instead of the expected `BrowserWindow`.

https://github.com/mozilla/addon-sdk/commit/f2c93bd8cac8c48ff11f6ae2f4ce84d99e037ded
Merge pull request #630 from ZER0/window.tabs/bug804935

Fix Bug 804935 - TypeError: window.tabs is undefined r=@erikvold
Target Milestone: --- → 1.13
Wes I should've marked this target as 1.12, but I let it slip.  We should get this regression fix released soon.
Flags: needinfo?(evold)
Attachment #674838 - Flags: review- → review+
Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/532a51a324d7d91a61099877977ae50591286580
Back out Merge pull request #630 from ZER0/window.tabs/bug804935 for causing debug failures.

This reverts commit f2c93bd8cac8c48ff11f6ae2f4ce84d99e037ded, reversing
changes made to b205f229a706aa8d8c622d4a3cacda5e0705e5ed.
erik, could you tell which kind of failures this patch is caused?

This patch just put back the original code that was removed during a regression, therefore I suppose a new code is interacting with that and causing problems?
Flags: needinfo?(evold)
Attached file pull 720
Attachment #674838 - Attachment is obsolete: true
Attachment #700720 - Flags: review?(zer0)
Flags: needinfo?(evold)
(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #23)
> erik, could you tell which kind of failures this patch is caused?

I didn't look too deeply at the error logs, but what I noticed was a lot of timeout errors.

> This patch just put back the original code that was removed during a
> regression, therefore I suppose a new code is interacting with that and
> causing problems?

The original code was using require('windows') during runtime instead of while loading which I suspect is the reason why your patch failed Matteo, so that is all that I've changed in my patch.  I spent sometime trying to prove this was the issue but I couldn't put my finger on the issue, so I'd have to spend more time.

If you're ok with the pull request that I've made, then maybe we could just try it and see if our debug builds start complaining again.
Erik, I review the pull and added a comment, nothing special but have a look if you can. Then let's merge it and see if everything works fine!
Attachment #700720 - Flags: review?(zer0) → review+
Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/b0a85503f5620de2413b971f89ebd0548d783e33
adding a test for bug 804935 window.tabs dne error

https://github.com/mozilla/addon-sdk/commit/a722b820818aa631ae81446e5322cc264f083590
Merge pull request #720 from erikvold/804935

Bug 804935 window.tabs is undefined r=@zer0
Commit pushed to stabilization at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/0abd820aac2ae966f56f749e51f51976e0f78934
adding a test for bug 804935 window.tabs dne error
(cherry picked from commit b0a85503f5620de2413b971f89ebd0548d783e33)
Commit pushed to stabilization at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/c1907229502ce8b39790da17ad6e1ae41680e907
Merge pull request #720 from erikvold/804935

Bug 804935 window.tabs is undefined r=@zer0(cherry picked from commit a722b820818aa631ae81446e5322cc264f083590)
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/49a60aedf612dd847242fe57c201335c2d012898
Adding a test for Bug 804935 worker.tab dnw when require(tab) was not used
Commit pushed to stabilization at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/49a60aedf612dd847242fe57c201335c2d012898
Adding a test for Bug 804935 worker.tab dnw when require(tab) was not used
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: