1156069 - promise chain failed involving getHiddenWindow

Status: RESOLVED FIXED

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

Description

[Adrian Aichner [:anaran]]

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:40.0) Gecko/20100101 Firefox/40.0
Build ID: 20150419030206

Steps to reproduce:

Deactivate and Activate a jetpack add-on


Actual results:

A promise chain failed to handle a rejection. Did you forget to '.catch', or did you forget to 'return'?
See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise

Date: Sun Apr 19 2015 16:32:37 GMT+0200 (CEST)
Full Message: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIAppShellService.hiddenDOMWindow]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/window/utils.js :: getHiddenWindow :: line 65"  data: no]
Full Stack: @undefined:65:undefined
getHiddenWindow@resource://gre/modules/commonjs/sdk/window/utils.js:65:3
@resource://gre/modules/commonjs/sdk/addon/window.js:20:20
startup/<@resource://gre/modules/commonjs/sdk/addon/runner.js:71:19
Handler.prototype.process@resource://gre/modules/Promise-backend.js:867:23
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise-backend.js:746:7
this.PromiseWalker.scheduleWalkerLoop/<@resource://gre/modules/Promise-backend.js:688:37
Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise-backend.js:688:5
this.PromiseWalker.schedulePromise@resource://gre/modules/Promise-backend.js:709:7
this.PromiseWalker.completePromise@resource://gre/modules/Promise-backend.js:671:7
readURI/<@resource://gre/modules/commonjs/sdk/net/url.js:55:9
NetUtil_asyncFetch2/<.onStopRequest@resource://gre/modules/NetUtil.jsm:212:17


Expected results:

No such error

Comment 1

[Richard Newman [:rnewman]]

The hidden window was removed in Bug 681733.

I don't see getHiddenWindow in the Add-on SDK docs, so perhaps it was correctly removed there, too?

Comment 2

[Dave Townsend [:mossop]]

We don't document many of the low level stuff we rely on to work. This probably means every SDK based add-on is busted on Fennec right now :(

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Can we find out what the hidden window is actually used for? And is it possible to stop using it in the Jetpack? Also, assuming we can remove this dependency, does this mean that all SDK-based add-ons will need to be rebuilt in order to work? (I'm not familiar with jetpack architecture - not sure if the bit of code using the hidden window is part of the add-on package or part of gecko).

Comment 4

[Erik Vold [:erikvold] (please needinfo? me)]

Hmm this means addon/window and page-worker apis are broken, the former is required on sdk/addon/runner which is breaking addon activation/deactivation.

I'm not sure we should remove the hidden window, I asked for this change to be reverted in bug 681733.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Can you explain or point to the code that uses the hidden window? Or otherwise answer my questions from comment 2?

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #5)
> Can you explain or point to the code that uses the hidden window? Or
> otherwise answer my questions from comment 2?

Used for some logging here:
http://mxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/addon/window.js#22

Looks like it used for real here:
http://mxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/addon/window.js#32

The comment above that says we can drop the hiddenWindow once bug 565388 is fixed, which it is, but it looks like we now depend on bug 946015, which is blocked on something else.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Curse us for knowing using the hiddenWindow, a practice that we have been telling add-on developers to not do since before I worked at Mozilla.

Comment 8

[Richard Newman [:rnewman]]

I am strongly disinclined to keep including an ancient misfeature that impacts our perf -- killing the hidden window is a five-digit bug!

I'm thus strongly disinclined to back out its removal, even if it (temporarily) breaks add-ons that use the Add-on SDK. Love add-ons though I do, they are a much less compelling argument for us than they are on desktop, especially when compared to perf. Perf impacts everyone.

Can we use this as the motivation to get Bug 946015 fixed?

Comment 9

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #5)
> Can you explain or point to the code that uses the hidden window? Or
> otherwise answer my questions from comment 2?

I tried to explain the issue in comment 5, what did you not understand?

All modules can be found here https://github.com/mozilla/addon-sdk/tree/master/lib/sdk

sdk/addon/runner is here https://github.com/mozilla/addon-sdk/blob/master/lib/sdk/addon/runner.js
sdk/page-worker is here https://github.com/mozilla/addon-sdk/blob/master/lib/sdk/page-worker.js

Comment 10

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Mark Finkle (:mfinkle) from comment #6)
> (In reply to Kartikaya Gupta (email:[e-mail]) from comment #5)
> > Can you explain or point to the code that uses the hidden window? Or
> > otherwise answer my questions from comment 2?
> 
> Used for some logging here:
> http://mxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/addon/
> window.js#22
> 
> Looks like it used for real here:
> http://mxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/addon/
> window.js#32
> 
> The comment above that says we can drop the hiddenWindow once bug 565388 is
> fixed, which it is, but it looks like we now depend on bug 946015, which is
> blocked on something else.

So bug 565388 probably shouldn't be marked as fixed, it looks like Irakli reopened it, but it isn't clear to me what was missing in order to use invisible docshells.

Comment 11

[Kartikaya Gupta (email:[e-mail])]

(In reply to Erik Vold [:erikvold] (please needinfo? me) from comment #9)
> (In reply to Kartikaya Gupta (email:[e-mail]) from comment #5)
> > Can you explain or point to the code that uses the hidden window? Or
> > otherwise answer my questions from comment 2?
> 
> I tried to explain the issue in comment 5, what did you not understand?

The code links helped, thanks. It's still not clear to me what the invisible docshell/hidden window frame is actually used for, but I guess I can look through more of the code and find out.

And I'm still not sure if existing addons would still be broken if we landed a change for the add-on sdk to not use the hidden window. Is the hidden window something we will need to keep around for backwards-compatibility with published addons indefinitely?

Given that we haven't seen any perf benefit from removing the hidden window (per IRC backscroll in #mobile last night) I don't strongly object to backing out that change but I would like to make sure we have a clear plan and dependencies identified to keep moving forward on this.

Comment 12

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

(In reply to Erik Vold [:erikvold] (please needinfo? me) from comment #10)
> (In reply to Mark Finkle (:mfinkle) from comment #6)
> > (In reply to Kartikaya Gupta (email:[e-mail]) from comment #5)
> > > Can you explain or point to the code that uses the hidden window? Or
> > > otherwise answer my questions from comment 2?
> > 
> > Used for some logging here:
> > http://mxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/addon/
> > window.js#22
> > 
> > Looks like it used for real here:
> > http://mxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/addon/
> > window.js#32
> > 
> > The comment above that says we can drop the hiddenWindow once bug 565388 is
> > fixed, which it is, but it looks like we now depend on bug 946015, which is
> > blocked on something else.
> 
> So bug 565388 probably shouldn't be marked as fixed, it looks like Irakli
> reopened it, but it isn't clear to me what was missing in order to use
> invisible docshells.

The issue with invisible docshells was that there were no observer notifications invoked for a documents tree with in it. This did not work for us as a lot of code hooks depend on certain observer notifications like document-element-inserted or dom-window-destroyed for example.

Comment 13

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

As of hidden window usage in SDK, we needed to have a window context as not all of the HTML APIs are exposed via XPCOM and in many cases even when they are exposed they still assume window and a domain. Gabor did a lot of work in making those APIs usable without a window context but still that was ton of work and new APIs had to be ported as well. As a stop gap solution we were creating an iframe with-in the hidden window that served as add-on window context from which we could expose things like `MessageChannel` for instance or `WebSocket` etc.. We also used that window context for things like `page-worker` API that creates an iframe with in that add-on window.

Given that porting each API was not working Gabor worked on invisible docshells so we could use that instead of hidden window hacks. But unfortunately it never got to a state where we could just switch to it for the lack of observer notifications dispatched with in that tree.

That being said it still maybe worth investigating what APIs break with invisible docshells patch and maybe use hidden window hack for only those. At least that would fix some APIs on fennec.

If someone could work on observer notifications for invisible docshells it would take almost no effort for us to ditch hidden window.

Comment 14

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

In fact here in Bug 946015 there is an attempt to switch to invisible dock shells and comments about the issues that prevented us from doing so.

Comment 15

[Kartikaya Gupta (email:[e-mail])]

Fixed by backout of bug 681733.