[Bug] ESNG does not initialize properly in some context(s?)

oleole39 commented 1 year ago

Hello @opto,

Good news, I am finally getting ESNG working again for me since I last reported a bug, thanks to the little fix described below.

1. Symptom

Now using TB 102.7.1 & ESNG 3.8.10, the issue was the same than at the time of the bug report mentionned, i.e. :

no ESNG search box displayed
nothing happening when clicking on the toolbar's "Expression" button (of which the icon had disappeared).
no tool icon on the add-on card displaying to set the extension's preferences

related console error message similar to the one shown in my previous bug report:

wind is null implementation.js:54
  onStartup jar:file:///{my_TB_profile_folder}/extensions/expressionsearch@opto.one.xpi!/api/ExpressionSearch/implementation.js:54
  onStartup resource://gre/modules/ExtensionCommon.jsm:1464
  openWindowPrompt resource://gre/modules/Prompter.jsm:1226
  openPrompt resource://gre/modules/Prompter.jsm:1067
  openPromptSync resource://gre/modules/Prompter.jsm:1046
  confirmEx resource://gre/modules/Prompter.jsm:1465
  confirmEx resource://gre/modules/Prompter.jsm:305
  onProfileStartup resource:///modules/OfflineStartup.jsm:94
  observe resource:///modules/OfflineStartup.jsm:123

2. Context

I eventually understood that this issue was only happening if TB preference offline.startup_state was set to 1 (i.e. show an prompt box at startup to ask whether to go online).

3. Problem

I don't really know what returns call such as Services.wm.getMostRecentWindow("mail:3pane") but my guess is that when making this call at startup in the given context, ESNG actually finds an object relative to the prompt box instead of the mail pane, resulting to a null value (it may be similar to the issue discussed there). This case is not foreseen in the onStartup() function which cannot handle the case and thus fails to initialize ESNG properly.

4. Proposed fix

In file /expressionsearch@opto.one.xpi/api/ExpressionSearch/implementation.js, replace line 53:

if (typeof (wind) != "undefined") {

with:

if (wind != null) { // checks that 'wind' is neither null nor undefined, cf. https://stackoverflow.com/questions/2559318/how-to-check-for-an-undefined-or-null-variable-in-javascript#answer-15992131

This seems to solve my issue.

Here is a packaged version of the fix if it can help some other people. Install instructions:

download,
change zip extension into xpi (Github wouldn't accept attaching xpi),
select "install from file" via Thunderbird

5. Going further

I notice there are in ESNG's code other checks for other contexts in which typeof(var) compares to "undefined" . Would there be some potential other bugs which would require the null case to be foreseen as well ?

quentinDupont commented 1 year ago

Hello and thank you for the work !!

I just tested with the zip/xpi. Works fine for the moment :tada:
I come back in a week to confirm this fix :)

oleole39 commented 1 year ago

Glad it made it for you too - as for me I confirm it solved my issue since my message above posted a month ago.

quentinDupont commented 1 year ago

Could it be added in official code then ? :)

oleole39 commented 1 year ago

Could it be added in official code then ? :)

That is to be done by the add-on developer, who I am not. I assume he had no to time review all this so far, but hopefully will before releasing a new version. Also this repository is set in a way which is not convenient to submit a pull request, as discussed in #50.

quentinDupont commented 1 year ago

Thanks, I understand. In any case, it works like a charm for me !

opto commented 1 year ago

Thanks for finding this. Services.wm.getMostRecentWindow("mail:3pane") actually hands over a reference to the main window, and it seems that does not yet exist with your startup option. But it should exist, so I think it is a TB issue. Thanks for finding the workaround, I will add that.

Klaus

Soon, TB 115 Supernova will come (in two weeks?). There, everything will be different again, as a lot of TB code has been rewritten. Therfore, the update to TB 115 is really a lot of work - donations are very much appreciated, see https://github.com/opto/Expression-Search-NG/issues/72

oleole39 commented 1 year ago

@opto

Thank you for considering that issue.

Thanks for finding the workaround, I will add that.

I updated today to ESNG 3.8.11 (dating July 3), and notice it does not take into account the workaround described hereinabove, thus breaking ESNG for me. I attach here a patched version for 3.8.11 (cf. the first message of that thread for diff and installation instructions).

oleole39 commented 9 months ago

Hello @opto, For the record this issue is still living even though the fix I proposed here above keeps working with TB 115.8.1 (just patched ESNG 4.0.25 with it and it works well). Would you mind applying it to your next releases?

oleole39 commented 5 months ago

@opto Still broken with ESNG 4.0.34 on TB 115.12.0. Above fix still works.

opto / Expression-Search-NG