Closed
Bug 804935
Opened 12 years ago
Closed 12 years ago
TypeError: window.tabs is undefined
Categories
(Add-on SDK Graveyard :: General, defect, P1)
Add-on SDK Graveyard
General
Tracking
(Not tracked)
RESOLVED
FIXED
1.13
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
Reporter | ||
Comment 1•12 years ago
|
||
The example was from bug 803529
Reporter | ||
Comment 2•12 years ago
|
||
and I get the error when opening a tab for erikvold.com
Reporter | ||
Comment 3•12 years ago
|
||
(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>
});
Reporter | ||
Comment 4•12 years ago
|
||
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);
}
});
Assignee | ||
Comment 5•12 years ago
|
||
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.
Comment 6•12 years ago
|
||
Surely this should cause tests to fail?
Assignee | ||
Comment 7•12 years ago
|
||
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.
Assignee | ||
Comment 8•12 years ago
|
||
“... and don't call me Shirley.” (Sorry I couldn't resist)
Assignee | ||
Comment 9•12 years ago
|
||
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.
Assignee | ||
Comment 10•12 years ago
|
||
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.
Reporter | ||
Comment 11•12 years ago
|
||
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 | ||
Updated•12 years ago
|
Assignee: nobody → zer0
Reporter | ||
Comment 12•12 years ago
|
||
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..
Reporter | ||
Comment 13•12 years ago
|
||
* `testGetTabForWindow` is in `test-tab.js`
Assignee | ||
Comment 14•12 years ago
|
||
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.
Assignee | ||
Comment 15•12 years ago
|
||
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.
Assignee | ||
Comment 16•12 years ago
|
||
Pointer to Github pull-request
Assignee | ||
Updated•12 years ago
|
Attachment #674838 -
Flags: review?(evold)
Priority: -- → P1
Reporter | ||
Comment 17•12 years ago
|
||
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
Reporter | ||
Updated•12 years ago
|
Flags: needinfo?(zer0)
Reporter | ||
Updated•12 years ago
|
Flags: needinfo?(zer0)
Reporter | ||
Updated•12 years ago
|
Attachment #674838 -
Flags: review?(evold) → review+
Reporter | ||
Updated•12 years ago
|
Attachment #674838 -
Flags: review+ → review-
Reporter | ||
Comment 18•12 years ago
|
||
(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.
Assignee | ||
Comment 19•12 years ago
|
||
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)
Comment 20•12 years ago
|
||
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
Reporter | ||
Updated•12 years ago
|
Target Milestone: --- → 1.13
Reporter | ||
Comment 21•12 years ago
|
||
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)
Reporter | ||
Updated•12 years ago
|
Attachment #674838 -
Flags: review- → review+
Comment 22•12 years ago
|
||
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.
Assignee | ||
Comment 23•12 years ago
|
||
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)
Reporter | ||
Comment 24•12 years ago
|
||
Attachment #674838 -
Attachment is obsolete: true
Attachment #700720 -
Flags: review?(zer0)
Flags: needinfo?(evold)
Reporter | ||
Comment 25•12 years ago
|
||
(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.
Assignee | ||
Comment 26•12 years ago
|
||
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!
Assignee | ||
Updated•12 years ago
|
Attachment #700720 -
Flags: review?(zer0) → review+
Comment 27•12 years ago
|
||
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
Comment 28•12 years ago
|
||
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)
Comment 29•12 years ago
|
||
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: 12 years ago
Resolution: --- → FIXED
Comment 30•12 years ago
|
||
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
Comment 31•12 years ago
|
||
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.
Description
•