Bug 62977 – Web Inspector: [V8] Tab crashes in chromium after opening inspector on about:blank.

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

[Vsevolod Vlasov]

Tab crashes in chromium after opening inspector on about:blank.

This was introduced in http://trac.webkit.org/changeset/89185 (https://bugs.webkit.org/show_bug.cgi?id=62653)

[Dmitry Lomov]

(In reply to comment #0)
> Tab crashes in chromium after opening inspector on about:blank.
> 
> This was introduced in http://trac.webkit.org/changeset/89185 (https://bugs.webkit.org/show_bug.cgi?id=62653)

It appears that showing inspector crashes for all pages that do not have JavaScript.
Preparing a patch.

[Pavel Feldman]

> It appears that showing inspector crashes for all pages that do not have JavaScript.
> Preparing a patch.

Please make sure you have a test along with the fix. Btw, having the stack trace in the [crash] bug would be great!

[Dmitry Lomov]

Created attachment 97849 [details]
Fix

V8BindingPerIsolate data was not initialized in debugger. This causes breakage if debugger script is the only javascript executing on the page (render process).

Not asking for cq yet: the change is still running through trybots.

I'd appreciate help writing unit-test for this (opening web inspector on a page without javascript). Where to look?

[Dmitry Lomov]

(In reply to comment #2)
> > It appears that showing inspector crashes for all pages that do not have JavaScript.
> > Preparing a patch.
> 
> Please make sure you have a test along with the fix. Btw, having the stack trace in the [crash] bug would be great!

I am not sure how to ASSERT with stacktrace from WebCore.

[Vitaly Repeshko]

It'd be nice to avoid adding scope objects that duplicate the functionality V8 provides natively. It's hard to enforce their usage in the bindings, because until we hit a corner case like this one everything will just happily work with plain handle and context scopes.

I see two ways out: make V8BindingPerIsolateData lazily initialized (if the perf impact of this is not measurable, it's probably the simplest thing here) or initialize it early (there used to be some WebScript initialization calls done from render_thread).

[Pavel Feldman]

Comment on attachment 97849 [details]
Fix

Sounds like Vitaly is saying r-. This seems fragile indeed: there is nothing preventing us from introducing another 'poor' call site. Other subsystems (such as content scripts on static pages) also seem vulnerable unless covered implicitly.

[Dmitry Lomov]

(In reply to comment #6)
> (From update of attachment 97849 [details])
> Sounds like Vitaly is saying r-. This seems fragile indeed: there is nothing preventing us from introducing another 'poor' call site. Other subsystems (such as content scripts on static pages) also seem vulnerable unless covered implicitly.

Yeah, I somehow thought that lazy initialization will be costly but my recent investigations suggest otherwise. I'll validate that and go with it if successful.

I do not think there is a good way to initialize V8 globally though.

[Vitaly Repeshko]

(In reply to comment #7)
> I do not think there is a good way to initialize V8 globally though.

There are calls to WebScriptController::enableV8SingleThreadMode(). Maybe we can add calls to e.g. WebScriptController::initialize() nearby (and a few more places)?

[Dmitry Lomov]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 97849 [details] [details])
> > Sounds like Vitaly is saying r-. This seems fragile indeed: there is nothing preventing us from introducing another 'poor' call site. Other subsystems (such as content scripts on static pages) also seem vulnerable unless covered implicitly.
> 
> Yeah, I somehow thought that lazy initialization will be costly but my recent investigations suggest otherwise. I'll validate that and go with it if successful.
> 
> I do not think there is a good way to initialize V8 globally though.

Lazy init is a noticable hit on Linux:
http://dromaeo.com/?id=142531,142743
So I'll try global initialization.

[Dmitry Lomov]

(In reply to comment #8)
> (In reply to comment #7)
> > I do not think there is a good way to initialize V8 globally though.
> 
> There are calls to WebScriptController::enableV8SingleThreadMode(). Maybe we can add calls to e.g. WebScriptController::initialize() nearby (and a few more places)?

Hmm interesting - what do you think about adding the initialization to WebKit::initialize?

[Vitaly Repeshko]

(In reply to comment #9)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > (From update of attachment 97849 [details] [details] [details])
> > > Sounds like Vitaly is saying r-. This seems fragile indeed: there is nothing preventing us from introducing another 'poor' call site. Other subsystems (such as content scripts on static pages) also seem vulnerable unless covered implicitly.
> > 
> > Yeah, I somehow thought that lazy initialization will be costly but my recent investigations suggest otherwise. I'll validate that and go with it if successful.
> > 
> > I do not think there is a good way to initialize V8 globally though.
> 
> Lazy init is a noticable hit on Linux:
> http://dromaeo.com/?id=142531,142743
> So I'll try global initialization.

Can you share the patch please?

[Vitaly Repeshko]

(In reply to comment #10)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > I do not think there is a good way to initialize V8 globally though.
> > 
> > There are calls to WebScriptController::enableV8SingleThreadMode(). Maybe we can add calls to e.g. WebScriptController::initialize() nearby (and a few more places)?
> 
> Hmm interesting - what do you think about adding the initialization to WebKit::initialize?

I think it's a good place. We should be careful to avoid initializing too much there as it can hurt startup time.

[Dmitry Lomov]

(In reply to comment #12)
> (In reply to comment #10)
> > (In reply to comment #8)
> > > (In reply to comment #7)
> > > > I do not think there is a good way to initialize V8 globally though.
> > > 
> > > There are calls to WebScriptController::enableV8SingleThreadMode(). Maybe we can add calls to e.g. WebScriptController::initialize() nearby (and a few more places)?
> > 
> > Hmm interesting - what do you think about adding the initialization to WebKit::initialize?
> 
> I think it's a good place. We should be careful to avoid initializing too much there as it can hurt startup time.

(In reply to comment #12)
> (In reply to comment #10)
> > (In reply to comment #8)
> > > (In reply to comment #7)
> > > > I do not think there is a good way to initialize V8 globally though.
> > > 
> > > There are calls to WebScriptController::enableV8SingleThreadMode(). Maybe we can add calls to e.g. WebScriptController::initialize() nearby (and a few more places)?
> > 
> > Hmm interesting - what do you think about adding the initialization to WebKit::initialize?
> 
> I think it's a good place. We should be careful to avoid initializing too much there as it can hurt startup time.

From what I see of the call-sites, V8 is already intialized implicitly by that time, so I wouldn't worry about it.

[Dmitry Lomov]

Created attachment 97885 [details]
Better fix

This eagerly initializes V8BindingPerIsolateData in WebKit::initialize.

[Dmitry Lomov]

Comment on attachment 97885 [details]
Better fix

(Clearing the cq since no results from trybots yet)

[Dmitry Lomov]

(In reply to comment #11)
> (In reply to comment #9)
> > (In reply to comment #7)
> > > (In reply to comment #6)
> > > > (From update of attachment 97849 [details] [details] [details] [details])
> > > > Sounds like Vitaly is saying r-. This seems fragile indeed: there is nothing preventing us from introducing another 'poor' call site. Other subsystems (such as content scripts on static pages) also seem vulnerable unless covered implicitly.
> > > 
> > > Yeah, I somehow thought that lazy initialization will be costly but my recent investigations suggest otherwise. I'll validate that and go with it if successful.
> > > 
> > > I do not think there is a good way to initialize V8 globally though.
> > 
> > Lazy init is a noticable hit on Linux:
> > http://dromaeo.com/?id=142531,142743
> > So I'll try global initialization.
> 
> Can you share the patch please?

The patch is pretty much this:
       static V8BindingPerIsolateData* get(v8::Isolate* isolate)
        {
            void* data = isolate->GetData();
            if (UNLIKELY(data == 0))
              return create(isolate);
            return static_cast<V8BindingPerIsolateData*>(data); 
        }

[Dmitry Lomov]

Created attachment 97908 [details]
Fixed workers initialization

[Dmitry Lomov]

Unit-test for this bug on chromium side: http://codereview.chromium.org/7215005/

[Dmitry Lomov]

Comment on attachment 97908 [details]
Fixed workers initialization

chromium trybots successful, adding cq?

[Yury Semikhatsky]

Comment on attachment 97908 [details]
Fixed workers initialization

View in context: https://bugs.webkit.org/attachment.cgi?id=97908&action=review

> Source/WebKit/chromium/public/WebScriptController.h:48
> +    WEBKIT_API static void initialize();

Is there a reason for exposing this method in WebKit API?

[Pavel Feldman]

Comment on attachment 97908 [details]
Fixed workers initialization

r- per Yury's comment. Could you please fix this ASAP as we might need to postpone Dev Channel release due to inspector not surviving the navigation.

[Timothy Hatcher]

Chromium and V8 have left the building. Won't fix.