Bug 26145 – WebInspector halts when opening with debugger enabled globally

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Pavel Feldman]

1. Open Web Inspector
2. Go to Scripts tab and choose 'Always enabled'
3. Restart browser
4. Open Web Inspector

Expected: window opens
Actual: window opens and gets into non-responsive mode. Browser halts.

[Pavel Feldman]

Created attachment 30909 [details]
fix

[Timothy Hatcher]

Comment on attachment 30909 [details]
fix

> -            enableDebugger();
> +            m_frontend->attachDebuggerWhenShown();

Can't you just leave this calling enableDebugger() since you fixed that function?

>          disableDebugger();
> +        m_attachDebuggerWhenShown = true;

Why is this needed? Shoudln't this be m_attachDebuggerWhenShown = false? If it is supose to be false, should disableDebugger() set it to false? If it isn't those things, maybe a comment is needed to explain.

> -        enableDebugger();
> +        m_attachDebuggerWhenShown = true;

Same as before. Can't you just leave this calling enableDebugger() since you fixed that function?

> +            this._attachDebuggerWhenShown = false;

I like to use delete this._attachDebuggerWhenShown for cases like this, since there is no benifit to keeping this property around.

r+, but maybe this can be a smaller change based on the comments above.

[Pavel Feldman]

Created attachment 30911 [details]
fix

(In reply to comment #2)
> (From update of attachment 30909 [details] [review])
> > -            enableDebugger();
> > +            m_frontend->attachDebuggerWhenShown();
> 
> Can't you just leave this calling enableDebugger() since you fixed that
> function?
> 

Done

> >          disableDebugger();
> > +        m_attachDebuggerWhenShown = true;
> 
> Why is this needed? Shoudln't this be m_attachDebuggerWhenShown = false? If it
> is supose to be false, should disableDebugger() set it to false? If it isn't
> those things, maybe a comment is needed to explain.
> 
> > -        enableDebugger();
> > +        m_attachDebuggerWhenShown = true;
> 

I added a comment for this. The scenario is: close window when debugger is enabled, expect it to be enabled when the window is being reopened. The hidden thing is that debugger is being disabled on window close unconditionally.

> Same as before. Can't you just leave this calling enableDebugger() since you
> fixed that function?
> 

Done. It is just that I fixed the function after I changed these.

> > +            this._attachDebuggerWhenShown = false;
> 
> I like to use delete this._attachDebuggerWhenShown for cases like this, since
> there is no benifit to keeping this property around.
> 

Done.

> r+, but maybe this can be a smaller change based on the comments above.
> 

Thanks!

[Brent Fulgham]

Assign for landing.

[Brent Fulgham]

Landed in @r44398.