Closed Bug 1286660 Opened 4 years ago Closed 4 years ago

JavaScript error: resource://gre/modules/FinderHighlighter.jsm, line 271: TypeError: controller is null

Categories

(Toolkit :: General, defect)

defect
Not set

Tracking

()

RESOLVED DUPLICATE of bug 1295759

People

(Reporter: ishikawa, Assigned: ishikawa)

References

Details

Attachments

(1 file)

After refreshing the source tree by |make -f client.mk checkout| and
building and running |make mozmill| test suite using locally built full debug version of C-C TB, I see many lines of

JavaScript error: resource://gre/modules/FinderHighlighter.jsm, line 271: TypeError: controller is null

in the test log, 130 times to be exact.
    130 JavaScript error: resource://gre/modules/FinderHighlighter.jsm, line 271: TypeError: controller is null

They seem to occur during teardown of the test (well most of them. There are cases that seem to be recorded in the middle of the test run.)

I did not see these errors on July 7th. So something has changed to introduce these errors.

File: mozilla/toolkit/modules/FinderHighlighter.jsm

This file seems to be actually rather new.

  261	
  262	  /**
  263	   * Clear all highlighted matches. If modal highlighting is enabled and
  264	   * the outline + dimmed background is currently visible, both will be hidden.
  265	   */
  266	  hide(window = null) {
  267	    window = window || this.finder._getWindow();
  268	
  269	    let doc = window.document;
  270	    let controller = this.finder._getSelectionController(window);
  271	    let sel = controller.getSelection(Ci.nsISelectionController.SELECTION_FIND);
  272	    sel.removeAllRanges();
  273	

Given that controller can be null when this function is called,
I think we should check it before 271 and return early if controller is null.

What do people think?
Component: Untriaged → General
Product: Thunderbird → Toolkit
The following patch introduced the code (and its push date July 7) matches
my observation well).

https://hg.mozilla.org/mozilla-central/rev/a06bc9ffb12c
Here is a suggested patch.

I could have return as soon as |controller| is null, but
there seems to be a few cleanup operation below and so used if-statementto avoid
reference via null |controller|.

I ran |make mozmill| test suite of C-C TB (using full debug build with -DDEBUG) and I think there are no new errors and confirm that the error message disappeared.

I put ralin@mozilla.com as the reviewer since the problem was introduced
by
https://hg.mozilla.org/mozilla-central/rev/a06bc9ffb12c
and its author was ralin@mozilla.com

TIA
Attachment #8771603 - Flags: review?(ralin)
Assignee: nobody → ishikawa
See Also: → 1198279
Comment on attachment 8771603 [details] [diff] [review]
Avoid reference through null |controller|

It looks good to me. There's no harm in adding a check here. 

Thank you ISHIKAWA.
Attachment #8771603 - Flags: review?(ralin) → review+
We would need another Firefox peer to review this, but Mike was OOO so you may need to find someone else.
Comment on attachment 8771603 [details] [diff] [review]
Avoid reference through null |controller|

Hi, Gijs

Could you help with this review? Thanks
Attachment #8771603 - Flags: review+ → review?(gijskruitbosch+bugs)
Comment on attachment 8771603 [details] [diff] [review]
Avoid reference through null |controller|

What's the stack for the error? Do we really need to call hide() in the cases where it is called and produces this error? If the controller and window are gone it seems like calling it is futile. A nullcheck wallpapers this, but without understanding why it's happening we could be glossing over actual bugs.
Flags: needinfo?(ishikawa)
Attachment #8771603 - Flags: review?(gijskruitbosch+bugs)
(In reply to :Gijs Kruitbosch from comment #6)
> Comment on attachment 8771603 [details] [diff] [review]
> Avoid reference through null |controller|
> 
> What's the stack for the error? Do we really need to call hide() in the
> cases where it is called and produces this error? If the controller and
> window are gone it seems like calling it is futile.

Actually, I thought it is futile, too, but just to be complete, I enclosed the problematic
part in if (controller) and get on with it.
At least, I don't see any new errors during |make mozmill| test suite run of C-C TB.

> A nullcheck wallpapers
> this, but without understanding why it's happening we could be glossing over
> actual bugs.

OK, so how do I get a stack trace?
The problem was noticed during a batch-like operation of |make mozmill| test suite run.
Is there a JavaScript-callable funciton to dump stacktrace?
If such a function exists, I can possibly insert it in
  if (controller) {
    ...}
  else
    call_this_stackrace_function() ; <--- here

to obtain the stacktrace during batch-like testing of |make mozmill|.

Actually, I myself want to figure out where a few errors occurred: many of the errors seem to happen near the "teardown", i.e., the termination time of TB, but a few of the errors were in the middle of tests, and we may have a reason to be concerned there.

TIA
Flags: needinfo?(ishikawa)
Cu.reportError("Str"); shows a stacktrace when run as part of a mochitest. I don't know about the mozmill tests. If that doesn't work,

dump(new Error().stack);

should work.
(In reply to :Gijs Kruitbosch from comment #8)
> Cu.reportError("Str"); shows a stacktrace when run as part of a mochitest. I
> don't know about the mozmill tests. If that doesn't work,
> 
> dump(new Error().stack);
> 
> should work.

Hi, I am trying to obtain the stack dump on a different PC: different from another PC where the errors were first observed more than a week ago. (That PC is offline due to relocation and won't be tinkered with earnestly until August 1st.)

Strange thing is that I did not see the error on this new computer. I have no idea why. 
Could it be that something has changed in the last 7 days or so and fixed the issue somehow?: that is, there is no longer a situation where |controller| can't be null in test environment?
[At the same time, I have a nagging feeling that there could be a timing issue since the errors I saw on the other PC happened, most of the time, near the termination time.]

Anyway, I have refreshed my source tree again and am retrying the test, but it may not be over until the weekend is over. 

I will keep you informed of the progress.

TIA


I suspect that the
(In reply to ISHIKAWA, Chiaki from comment #9)
> (In reply to :Gijs Kruitbosch from comment #8)
> > Cu.reportError("Str"); shows a stacktrace when run as part of a mochitest. I
> > don't know about the mozmill tests. If that doesn't work,
> > 
> > dump(new Error().stack);
> > 
> > should work.
> 
> Hi, I am trying to obtain the stack dump on a different PC: different from
> another PC where the errors were first observed more than a week ago. (That
> PC is offline due to relocation and won't be tinkered with earnestly until
> August 1st.)
> 
> Strange thing is that I did not see the error on this new computer. I have
> no idea why. 
> Could it be that something has changed in the last 7 days or so and fixed
> the issue somehow?: that is, there is no longer a situation where
> |controller| can't be null in test environment?

I don't know. The main person to do with find bar these days is Mike de Boer and he's on PTO, so it seems unlikely, but it's still possible.
The code seems to have been edited in the last several days, and I will see if I get the similar error with the new code. (I noticed the change because he tentative patch I posted doesn't apply any more.)
(In reply to ISHIKAWA, Chiaki from comment #11)
> The code seems to have been edited in the last several days, and I will see
> if I get the similar error with the new code. (I noticed the change because
> he tentative patch I posted doesn't apply any more.)

Any more news? Maybe we should close as incomplete and reopen if/when this happens again?
Flags: needinfo?(ishikawa)
Hi, I have been on a summer holiday break. On August 10, I still get warnings, but from a different place.
I will try to upload the latest log from the latest source (by refreshing the tree), and report back later today.
Flags: needinfo?(ishikawa)
On one of the test PC to which I have immediate access, I refreshed the source code a few hours ago, 
and recompiled and ran |make mozmill| test suite, and I still see the errors.

It occurs in _clearSelection(...)

I put the error dump statements in the code, but please see below.
|controller| can be null in the next line.

let sel = controller.getSelection(Ci.nsISelectionController.SELECTION_FIND);


  /**
   * Utility; removes all ranges from the find selection that belongs to a
   * controller. Optionally skips a specific range.
   *
   * @param  {nsISelectionController} controller
   * @param  {nsIDOMRange}            skipRange
   */
  _clearSelection(controller, skipRange = null) {
    if (!controller) {
      Cu.reportError("_clearSelction called with null controller");
      dump(new Error().stack);
      return;
    }
    let sel = controller.getSelection(Ci.nsISelectionController.SELECTION_FIND);
    if (!skipRange) {
      sel.removeAllRanges();

130 error messages as I originally noted.

TIA
(In reply to ISHIKAWA, Chiaki from comment #14)
> On one of the test PC to which I have immediate access, I refreshed the
> source code a few hours ago, 
> and recompiled and ran |make mozmill| test suite, and I still see the errors.
> 
> It occurs in _clearSelection(...)
> 
> I put the error dump statements in the code, but please see below.
> |controller| can be null in the next line.
> 
> let sel = controller.getSelection(Ci.nsISelectionController.SELECTION_FIND);
> 
> 
>   /**
>    * Utility; removes all ranges from the find selection that belongs to a
>    * controller. Optionally skips a specific range.
>    *
>    * @param  {nsISelectionController} controller
>    * @param  {nsIDOMRange}            skipRange
>    */
>   _clearSelection(controller, skipRange = null) {
>     if (!controller) {
>       Cu.reportError("_clearSelction called with null controller");
>       dump(new Error().stack);
>       return;
>     }
>     let sel =
> controller.getSelection(Ci.nsISelectionController.SELECTION_FIND);
>     if (!skipRange) {
>       sel.removeAllRanges();
> 
> 130 error messages as I originally noted.
> 
> TIA

So what was the stack you got from this?
Flags: needinfo?(ishikawa)
Mike, do you have ideas about this patch and whether it is the "correct" solution? Feels like you'd be quicker at iterating on this now that you're back.
Flags: needinfo?(mdeboer)
Thanks for filing this!
I'm touching this code in bug 1295759 in such a way that this won't happen again. It will be uplifted to Fx 50.
Status: NEW → RESOLVED
Closed: 4 years ago
Flags: needinfo?(mdeboer)
Flags: needinfo?(ishikawa)
Resolution: --- → DUPLICATE
Duplicate of bug: 1295759
(In reply to Mike de Boer [:mikedeboer] from comment #17)
> Thanks for filing this!
> I'm touching this code in bug 1295759 in such a way that this won't happen
> again. It will be uplifted to Fx 50.
> 
> *** This bug has been marked as a duplicate of bug 1295759 ***

Thank you for your debugging efforts.
(So I won't need to upload the stack trace, I suppose. If you need one, just ping me.)

TIA
You need to log in before you can comment on or make changes to this bug.