Inactive Tabs Weren't Faded After FF 6 Upgrade about bartablite

[inverseparadox]

I've been working on a BarTab Lite fork, intended to restore as much as possible of the extra functionality of the old non-Lite BarTab, and I see this same behavior through at least Firefox 21.

(As a side note, the tabs aren't actually being "made inactive by BarTab Lite"; they're being made inactive by max_concurrent_tabs=0 or by restore_on_demand=true, i.e. by Firefox itself.)

From what I can see based on extensive reportError message insertion, the problem is that on browser upgrade, documentElement.getAttribute("windowtype") is never equal to "navigator:browser" in utils.js:watchWindows:watcher() at a time when the function gets called. The code flow does get to that if(), but it never succeeds. As a result, loadIntoWindow() never gets called.

What I don't see, however, is why that happens. My only guess is that the addon's startup() gets called at a point where no window matches navigator:browser (e.g. when the Add-On Compatibility Wizard is the only thing open), and when a window does appear, somehow it doesn't trigger the notification for windowWatcher. That seems incompatible with the documented behavior of registerNotification(), however - unless for some reason the fact that windowWatcher doesn't implement an observe() sub-method matters in that case, even though it appears to work fine in other cases as-is.

This is just about the only important bug left in the fork that I know of, and I'm really reluctant to release with it still present. Any ideas about how to address it - or even investigate it further - are welcome.

[inverseparadox]

When I open the Error Console after a browser upgrade, I see the ReportError messages I inserted into pullstarter.js:watchWindows() (which contains much the same functions as utils.js, and performs the same service) which were triggered by opening the Error Console window.

However, the Error Console at that point does not contain any of the ReportError messages I inserted into bootstrap.js:startup(). Is the Error Console not initialized by the time the browser starts loading add-ons? If so, I need an alternate way of getting messages out in order to debug this further...

[xabolcs]

... at that point does not contain any of the ReportError messages I inserted into ...

In that case I prefer dump.

[inverseparadox]

On 06/24/2013 05:42 AM, Szabolcs Hubai wrote:

... at that point does not contain any of the ReportError messages
I inserted into ...

In that case I prefer dump.

That looks good, except that I don't have the 'window' object at the
points where I need to output messages (in the startup() function), and
I don't see a practical way of getting it.

The following code, when run at the beginning of startup():
let windows = Services.wm.getEnumerator(null);
if(windows.hasMoreElements) {
window = windows.getNext();
}
results in an Error Console warning on browser launch, and the failure
of the add-on to load; window is null at that point.

Since this is fundamentally the same code as gets run in
pullstarter.js:watchWindows(), that might help explain the original
problem: as far as the service is concerned, there literally are no
windows at the point where we're looking for them. That doesn't explain
why we don't get alerted later by registerNotification when windows do
show up, however.

Disabling and re-enabling the add-on at a later point doesn't produce
the same warning/failure, and with the appropriate about:config pref
set, the messages do make it to the console. That doesn't help much,
though, since reportError was already working at that point.

I wouldn't be surprised if it might help to do something like the
following (in pullstarter.js:watchWindows()):

if(!windows.hasMoreElements()) {
// wait one second and retry
window.setTimeout(watchWindows(type, callback), 1000);
} else {
while(windows.hasMoreElements()) {
// ... the rest of the original watchWindows() code ...
}

Except, of course, that if we already had a window object to call
setTimeout from we probably wouldn't need to try again.

[xabolcs]

... I don't have the window object ...

dump is everywhere! :)
Try it!

[inverseparadox]

On 06/24/2013 09:25 AM, Szabolcs Hubai wrote:

... I don't have the window object ...

dump is everywhere! :)
Try it!

So it is. The fact that the API documentation lists it as window.dump
without clearly mentioning this fact is therefore misleading. This does
make for a highly useful debugging tool, though.

There appear to be two problems.

One, when PullStarter calls windows.HasMoreElements() during initial
launch on browser upgrade or downgrade, it returns false immediately. At
this point, there are no windows with the requested type.

Two, for some yet-undetermined reason (possibly related to the reason
for the other problem), requestNotification doesn't let us know when a
new window (which does have the requested type) appears. This happens
even if the requestNotification call occurs before the original
hasMoreElements() loop, so it's not just a race condition.

That said, I've gotten it working by the following procedure:

• Use Services.ww.getWindowEnumerator instead of
  Services.wm.getEnumerator.
• Manually filter for the correct window type (since getWindowEnumerator
  doesn't provide a filter of its own).
• If we didn't get any window of the correct type, wait 10ms and try the
  entire watchWindows function again, using window.setTimeout() from the
  last window we did get.

The timeout appears to fire only once, even at that short of a delay,
and at least on my test VM, the resulting effect is as near seamless as
makes no never-mind. The minimum delay for setTimeout is 4ms, but I
don't know if it's worth dropping down to that point.

The only potential problem I see with this is that if there are
consistently no windows of any type to get, we will retry indefinitely.
However, I'm pretty sure that if that happens at a point where an add-on
would be initializing, Firefox has bigger problems; we probably don't
need to worry about it.

Patch pending, once I clear out all my debugging statements and make
sure the patch applies cleanly to upstream. More extensive and more
exhaustive testing is more than welcome.

Do you think it might be worthwhile to try to report a possible bug
against requestNotification, based on the fact that it doesn't provide
notification in this case?

I'm pretty sure the most it would get us is an explanation of why the
current behavior is correct (i.e. what's actually happening under the
hood), but even that might be useful to know.

[xabolcs]

Thanks for the detailed report.
I hope there is a stable resolution without setTimeout. :)

By the way did you know that, if you install Skip Addon Check, then the Update UI doesn't show up,
and all goes well?!
OMGWTF, isn't it? :)
For example AMI_startup and XPI_startup methods are very interesting to me.

[xabolcs]

Digging on mxr.m.o and playing with Services.obs.addObserver(PullStarter, "*", false); I found that sessionstore-windows-restored topic is always notified.
I'm trying to use that topic to run when upgrade happens.

[xabolcs]

Awesome!
Services.wm.getEnumerator(null) solves the problem in pullstarter.js for my BarTab Lite X.

Interestingly it's already null here, in utils.js!
And I can't reproduce with non-empty profile + 1.3 BarTab Lite.

But I'm sure, this was an issue with older Firefoxes.

[inverseparadox]

On 06/24/2013 07:43 PM, Szabolcs Hubai wrote:

Awesome! Services.wm.getEnumerator(null) solves the problem in
pullstarter.js for my BarTab Lite X.

Judging by the documentation, wm.getEnumerator(null) should enumerate
all windows exactly the same way as ww.getWindowEnumerator() does, so I
considered this during my commutes to and from work today, as a way of
trying to reduce the changes being made by the patch.

However, since in my testing ww.getWindowEnumerator() doesn't actually
return any windows of the requested type, I don't think this is enough
on its own. I think we do need the retry.

Fortunately, I think I see a way to make it safe - in terms of avoiding
an endless retry loop when called with a window type which is not
guaranteed to appear eventually, or even in a reasonable time-frame -
even with the retry; I'm planning to work on that in the morning.

Interestingly it's already null here, in utils.js!
And I can't reproduce with non-empty profile + 1.3 BarTab Lite.

But I'm sure, this was an issue with older Firefoxes.

I'll try to test it out when I get a chance; I have two separate and
unrelated testing environments (one at work and one at home), and I can
generally test older versions fairly easily in either one.

[inverseparadox]

Hmm. When I test with just wm.getEnumerator(null) as you suggest, rather than with the full set of changes from my setTimeout-based patch, it does indeed work.

But when I tested earlier with wm.getEnumerator(null) at the very beginning of startup() (rather than near the end of if, which is where watchWindows() gets called), it returned no windows at all.

So even though it works, I'm not sure if we can trust it to always work, rather than failing as a race condition in some cases. Still, probably better to make the more minimal change for now, and only fall back to the more intrusive one if we do see cases where this fails.

I'll test with the utils.js variant, and older Firefoxes (how old do we want to use?), when I get time. Right now, I need to get breakfast.

[xabolcs]

This issue is reproducible with version 1.2.

After that, commit f0d0244 landed which rewrites utils.js.
It uses Services.wm.getEnumerator(null) ... and fixes the problem, as stated above.
See 1.2 ... 1.3 compare view!

So even though it works, I'm not sure if we can trust it to always work, rather than failing as a race condition in some cases.

I really don't understand why does it fix, so yes, +1 for this.

[xabolcs]

OMG!

This is a DUPLICATE of mozilla/prospector#442!
See message from commit mozilla/prospector@7c24f48 below!

Closes mozilla/prospector#442: Add-on is enabled but doesn't load when Firefox upgrades

Watch all windows that haven't loaded to check if it becomes a navigator:browser as the watcher's notification is too late and mediator's enumerator doesn't match.

Now I understand why the fix (wm.getEnumerator(null)) works. And now I am sure it will always work.

[inverseparadox]

Ah. Reading the code, that makes sense, and I do understand why it works now; the trick is in the runOnLoad(), which already uses a setTimeout.

I'm still not positive it will always work; the failure case I was worried about was the one where wm.getEnumerator(null) returns no windows, as I believe I saw happen when trying to get a window object in startup() when I thought I needed one for window.dump(). Still, I've got no evidence that it will ever fail that way in practice, and I'm satisfied with this change as a fix.

This should probably get ported across to philikon/pullstarter, if possible. Do you want to handle that?

[xabolcs]

... the trick is in the runOnLoad(), which already uses a setTimeout. ...

I see only addEventlistener / removeEventListener there ...

I'm still not positive it will always work; the failure case I was worried about was the one where wm.getEnumerator(null) returns no windows, as I believe I saw happen when trying to get a window object in startup() when I thought I needed one for window.dump(). Still, I've got no evidence that it will ever fail that way in practice, and I'm satisfied with this change as a fix.

You're right, if you ask for window in startup at Application startup (reason == APP_STARTUP) , then it won't found any.
In this case Services.ww.registerNotification(windowWatcher); will do the job.
Try it! Put this line right after function windowWatcher(subject, topic) {:

  dump("watchWindows - new window opened!\n");

This should probably get ported across to philikon/pullstarter, if possible. Do you want to handle that?

Yeah, will do, once we finish this up.