The default bug view has changed. See this FAQ.

Add-on SDK is blocking assignment of tree element's view in non-browser window prior to initial browser window displaying

RESOLVED FIXED

Status

Add-on SDK
General
P2
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: morac, Assigned: ochameau)

Tracking

(Depends on: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
I've had a number of people using my Session Manager (https://addons.mozilla.org/en-US/firefox/addon/session-manager) add-on complain that it's opening a blank Session Manager window at browser start-up instead of the normal looking one.  Looking at logging data I've found this is because the tree element's view is null in the session manager window which causes an exception in my code.  I found this odd since it's set right before it's used.

I've determined that everyone having this problem is using some add-on that uses the Add-on SDK.  For example, there's an add-on Proxmate (https://addons.mozilla.org/en-US/firefox/addon/proxmate/) which uses the SDK.  When that is installed, the following code in Session Manager doesn't work at startup:

    this.gSessionTree.view = this.sessionTreeView;

After this line of code, this.gSessionTree.view is null despite this.sessionTreeView being defined (hard coded) and this.gSessionTree being a tree element.  This only happens when the browser first starts up as Session Manager opens a window prior to the browser window displaying and waits for user input.

I tracked the problem code in the add-on to the following.  If it is commented out, Session Manager displays correctly:

		// Widget initialisieren
		var statusButton = require("widget").Widget({
			id: "open-proxmate-btn",
			label: "Click to Activate/Deactivate Proxmate",
			contentURL: selfData.url("images/icon16.png"),
			onClick: setPluginStatus
		});


Actually I found if I removed everything from the main.js from that add-on and simply put in the following single line,

   require("widget");

the same problem occurs so it appears to be an issue with loading the widget component, prior to the browser window actually being displayed.

There was another add-on that used the SDK that someone had an issue with, but I can't remember the name of the add-on.  So far though the problem only occurs with add-ons that use the add-on SDK.

I haven't traced what the add-on SDK is doing, but I'm guessing it's somehow injecting itself into the Session Manager window and messing up the default nsITreeView processing.
(Reporter)

Comment 1

5 years ago
Actually I'm also finding that elements in the main browser window that should exist when the "onload" procedure is called are returning null, which also is wrong.
(Reporter)

Comment 2

5 years ago
One quick update.  The problem happens on the first browser window that opens after the add-on bootstrap startup method.  This also appears to affect browser JavaScript variables such as "gBrowser".  They indicate as not being defined for windows opened right after the add-on is enabled.

For example:

Error: ReferenceError: gBrowser is not defined
Source file: chrome://sessionmanager/content/sessionmanager.js
Line: 370
(Reporter)

Comment 3

5 years ago
Ignore comment #1 and #2.  It looks like the SDK is causing the chrome://browser/content/hiddenWindow.xul to load for some reason on Windows, which is what is causing those errors.  That doesn't explain the original problem I reported in comment #0 though.
Alex, can you take a look at this?
Assignee: nobody → poirot.alex
Whiteboard: [triage:followup]
(Assignee)

Comment 5

5 years ago
Still don't know why it fails so badly, but here is the code that break your addon:

In api-utils/tabs/observer.js: (This module is indirectly used by widget module)

  const { browserWindowIterator } = require("api-utils/window-utils");
  for each (let window in browserWindowIterator()) ;

And it is more precisely due to `isBrowser` method used internaly in browserWindowIterator method. Here is a minimal testcase that expose this issue.

  const { windowIterator } = require("api-utils/window-utils");
  function isBrowser(window) {
    window.document.documentElement;
  };
  for each (let window in windowIterator())
    isBrowser(window);

So the simple fact of accessing the documentElement of the document provoke this bug. (It pass if you only access to `window.document`.

We now have to find why is it failing?!
(Assignee)

Comment 6

5 years ago
Created attachment 623729 [details]
Pull request 436

I finally found why the DOM is broken. We are trying to access DOM while document is still loading in its early stages (i.e. "uninitialized" readyState).
If we stop doing that, it doesn't break your addon anymore.
Attachment #623729 - Flags: review?(rFobic)
(Assignee)

Comment 7

5 years ago
dcm: mossop: This bug is pretty bad!
Many jetpack addons can break some specific xul addons. Here we know that Session Manager (250k users!) face this issue. There is many chances that bug 739388 is a duplicate of this one.

Faulty jetpack addons are:
Addons using require(api-utils/window-utils).browserWindowIterator(). It means at least all addons using Widget, Windows or Tabs APIs.

Broken XUL addons:
Needs to be confirmed, but it seems to break addons opening a XUL window during simultaneous addon loading on firefox startup.

Result for users:
Buggy window that is blank, empty or partially working.

Solution:
Repack jetpack addons with an SDK including patch from comment 6.
We may fill a platform bug in order to see if this behavior is expected.
(Reporter)

Comment 8

5 years ago
I'll mention that I've come up with a work around for Session Manager (initialize it's XUL window from a component instead of a browser window) which I'm going to implement since even if this was fixed today, all addons using the SDK would need to be updated to use the fixed SDK.   It should still be fixed though.  I'd recommend coming up with a test case that doesn't involve Session Manager though.
(Assignee)

Comment 9

5 years ago
(In reply to Michael Kraft [:morac] from comment #8)
> I'll mention that I've come up with a work around for Session Manager
> (initialize it's XUL window from a component instead of a browser window)
> which I'm going to implement since even if this was fixed today, all addons
> using the SDK would need to be updated to use the fixed SDK.   It should
> still be fixed though.  I'd recommend coming up with a test case that
> doesn't involve Session Manager though.

Is Session Manager an open source software? If yes, do you have a link to this workaround modification? I still don't know why the DOM is broken, your workaround may help understanding why!
(Reporter)

Comment 10

5 years ago
The source is available at https://addons.mozilla.org/en-US/firefox/files/browse/150806/ but it's not exactly straightforward. 

The change I made was to open my dialog window from a "domwindowopened" observer notification in a component of mine, instead of from the window "load" event.  Basically I display the dialogue prompt prior to the browser window loading.   This works in Firefox 12.   I could also have opened it after the browser window, but I wanted to open it before hand. 

Note, my source also has another fix I added, prior to coming up with the component fix idea.  That simply detects the broken browser DOM and closes and reopens the window (which seems to work). That was an ugly fix though since the user could see the window open, close and open again.
(Assignee)

Comment 11

5 years ago
Created attachment 624067 [details]
Jetpack unit test against this <tree>.view failure

It took me a while to strip down your addon, but here is a minimal testcase highlighting the issue your are facing.
So it seems to require a bunch of condition in order to face it!
1/ Jetpack code has to access a <dialog> node while it is being loaded
2/ A tree node should be placed after a <script> tag 
3/ A script has to replace the tree view with a custom object
The final bug is that this custom object isn't correctly set and the view end up being null.
(Assignee)

Comment 12

5 years ago
dcm: Mossop: With this new testcase highlighting necessary conditions to face this bug, we may lower its severity. I thought it was easier to trigger, and that it broke multiple DOM features. It seems to break only `view` attribute for <xul:tree> node (with various conditions).

Michael: Can you confirm that this view attribute is the only one having a buggy behavior? It would be different if some other DOM features are broken by this windowIterator code!
(Reporter)

Comment 13

5 years ago
That's the only field I've seen a problem on.  Basically when I set the view to a hard coded object via script, it remains null.
(Assignee)

Updated

5 years ago
Depends on: 755380
Comment on attachment 623729 [details]
Pull request 436

Details are in associated pull request.
Attachment #623729 - Flags: review?(rFobic) → review-

Updated

5 years ago
Priority: -- → P2

Updated

5 years ago
Whiteboard: [triage:followup]
(Assignee)

Comment 15

5 years ago
Comment on attachment 623729 [details]
Pull request 436

I've updated the pull request, can you take another look?
Attachment #623729 - Flags: review- → review?(rFobic)
Attachment #623729 - Flags: review?(rFobic) → review+

Comment 16

5 years ago
Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/a2679588f0b73d0580683bfd4758adfb6fc0b730
Bug 752631: Process only fully loaded windows in api-utils/window-utils iterators.

https://github.com/mozilla/addon-sdk/commit/cc99d4b749adf38f2328867fad02ee7d6ec97556
Merge pull request #436 from ochameau/bug/752631-fix-browserWindowIterator

Bug 752631: Process only fully loaded window in api-utils/window-utils iterators. r=@gozala
(Assignee)

Updated

5 years ago
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 17

5 years ago
I just posted a comment about this eventual breaking change on the ML

Comment 18

5 years ago
Commits pushed to stabilization at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/a2679588f0b73d0580683bfd4758adfb6fc0b730
Bug 752631: Process only fully loaded windows in api-utils/window-utils iterators.

https://github.com/mozilla/addon-sdk/commit/cc99d4b749adf38f2328867fad02ee7d6ec97556
Merge pull request #436 from ochameau/bug/752631-fix-browserWindowIterator

Comment 19

5 years ago
Commits pushed to release at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/a2679588f0b73d0580683bfd4758adfb6fc0b730
Bug 752631: Process only fully loaded windows in api-utils/window-utils iterators.

https://github.com/mozilla/addon-sdk/commit/cc99d4b749adf38f2328867fad02ee7d6ec97556
Merge pull request #436 from ochameau/bug/752631-fix-browserWindowIterator
You need to log in before you can comment on or make changes to this bug.