1040751 - CodeMirror editor.destroy() isn't e10s compatible

Status: RESOLVED WORKSFORME

(DevTools :: Source Editor, defect)

Description

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

The following listeners do not get removed when CodeMirror's editor.destroy() is called in e10s mode:
      this._prefObserver.off(TAB_SIZE, this.reloadPreferences);
      this._prefObserver.off(EXPAND_TAB, this.reloadPreferences);
      this._prefObserver.off(KEYMAP, this.reloadPreferences);
      this._prefObserver.off(AUTO_CLOSE, this.reloadPreferences);
      this._prefObserver.off(AUTOCOMPLETE, this.reloadPreferences);
      this._prefObserver.off(DETECT_INDENT, this.reloadPreferences);

This prevents the next line being called, which is:
      this._prefObserver.destroy();

Comment 1

[Chris Peterson [:cpeterson]]

These DevTools bugs should probably block e10s from riding to Aurora.

Comment 2

[Chris Peterson [:cpeterson]]

devtools bugs will block the e10s merge to Aurora, but blassey would like them to be tracked by dte10s meta bug 875871, not the tracking-e10s=m6+ flag.

Comment 3

[Brian Grinstead [:bgrins]]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #0)
> The following listeners do not get removed when CodeMirror's
> editor.destroy() is called in e10s mode:
>       this._prefObserver.off(TAB_SIZE, this.reloadPreferences);
>       this._prefObserver.off(EXPAND_TAB, this.reloadPreferences);
>       this._prefObserver.off(KEYMAP, this.reloadPreferences);
>       this._prefObserver.off(AUTO_CLOSE, this.reloadPreferences);
>       this._prefObserver.off(AUTOCOMPLETE, this.reloadPreferences);
>       this._prefObserver.off(DETECT_INDENT, this.reloadPreferences);
> 
> This prevents the next line being called, which is:
>       this._prefObserver.destroy();

I wonder if we actually need to manually call off on all of these before calling destroy, or if destroy will handle that for us.

Comment 4

[Brian Grinstead [:bgrins]]

How can this be tested?  Based on the description it sounds like it maybe one of those lines is throwing an exception, causing the destroy function to not fire.  But I'm not seeing any errors in the Browser Console when opening and closing the Style Editor.

Comment 5

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Brian Grinstead [:bgrins] from comment #4)
> How can this be tested?  Based on the description it sounds like it maybe
> one of those lines is throwing an exception, causing the destroy function to
> not fire.  But I'm not seeing any errors in the Browser Console when opening
> and closing the Style Editor.

No idea, maybe try / catch, using dump between each line, stepping through the code etc.

Comment 6

[Brian Grinstead [:bgrins]]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #5)
> (In reply to Brian Grinstead [:bgrins] from comment #4)
> > How can this be tested?  Based on the description it sounds like it maybe
> > one of those lines is throwing an exception, causing the destroy function to
> > not fire.  But I'm not seeing any errors in the Browser Console when opening
> > and closing the Style Editor.
> 
> No idea, maybe try / catch, using dump between each line, stepping through
> the code etc.

There are no errors in the editor.destroy function, it runs through all the way to the end.  And there don't seem to be any leaks.  So unless if you remember anything else specifically about this issue, going to resolve.