Bug 27906 – [chromium] MessagePort construction fails in worker process

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[John Abd-El-Malek]

Need a fix in bindings.  Per discussion with Dimitri, here's the fix and cleanup of the other place.

[John Abd-El-Malek]

Created attachment 33915 [details]
Proposed patch

[John Abd-El-Malek]

note, for the MessagePort case, I hit this assert in debug mode and I'm not sure what the right fix is (i.e. this isn't ready to commit until it's fixed)

WorkerContextExecutionProxy::ToV8Object calls
V8DOMWrapper::setJSWrapperForDOMObject which has this code:

#ifndef NDEBUG
    V8ClassIndex::V8WrapperType type = V8DOMWrapper::domWrapperType(wrapper);
    switch (type) {
#define MAKE_CASE(TYPE, NAME) case V8ClassIndex::TYPE:
        ACTIVE_DOM_OBJECT_TYPES(MAKE_CASE)
        ASSERT_NOT_REACHED();
#undef MAKE_CASE
    default:
        break;
    }
#endif

Note sure why the check was added.  Should WOrkerContextExecutionProxy not call V8DOMWrapper::setJSWrapperForDOMObject(impl, result); for MessagePort?

[John Abd-El-Malek]

Created attachment 33918 [details]
fix style problems

[Dimitri Glazkov (Google)]

Comment on attachment 33918 [details]
fix style problems

r=me.

I feel a bit queasy looking WorkerContextExecutionProxy and all its parallel methods. It also needs to be style-cleaned. I'll aks jianli or dimich to look into this.

[John Abd-El-Malek]

Thanks, I agree it'd be good to try and share more code there.
per comment 2, this is NOT ready to commit.  There's still an assert that gets hit, I was wondering if you know the best way to avoid it (not sure exactly what it's testing against).  I'll ping you on irc in a little bit, gotta take care of some other stuff first.

[John Abd-El-Malek]

Created attachment 33983 [details]
Fixed a few problems with WorkerContextExecutionProxy, good to go

[Dimitri Glazkov (Google)]

Comment on attachment 33983 [details]
Fixed a few problems with WorkerContextExecutionProxy, good to go

Ugh. r=me.

[David Levin]

Drive by comments, 

In WebCore/bindings/v8/V8DOMWrapper.cpp

 520     // These objects can be constructed under WorkerContextExecutionProxy, need special handling
 521     // as V8Proxy::retrieve() will crash then.

I can't parse this (maybe there is a typo).

Line 523-526 seems like it should be indented by one more space.



In WebCore/bindings/v8/WorkerContextExecutionProxy.cpp,

219 #define MAKE_CASE(TYPE, NAME) case V8ClassIndex::TYPE:
 220         ACTIVE_DOM_OBJECT_TYPES(MAKE_CASE)
 221         isActiveDomObject = true;
 222 #undef MAKE_CASE


Shouldn't there be a break right after line 221 (or else something that says fall through)?

[John Abd-El-Malek]

Created attachment 33985 [details]
fixed David's comments

[John Abd-El-Malek]

(In reply to comment #8)
> Drive by comments, 

Thanks, replies below.

> 
> In WebCore/bindings/v8/V8DOMWrapper.cpp
> 
>  520     // These objects can be constructed under WorkerContextExecutionProxy,
> need special handling
>  521     // as V8Proxy::retrieve() will crash then.
> 
> I can't parse this (maybe there is a typo).

you're right, it wasn't that clear.  updated.
> 
> Line 523-526 seems like it should be indented by one more space.
> 

done

> 
> 
> In WebCore/bindings/v8/WorkerContextExecutionProxy.cpp,
> 
> 219 #define MAKE_CASE(TYPE, NAME) case V8ClassIndex::TYPE:
>  220         ACTIVE_DOM_OBJECT_TYPES(MAKE_CASE)
>  221         isActiveDomObject = true;
>  222 #undef MAKE_CASE
> 
> 
> Shouldn't there be a break right after line 221 (or else something that says
> fall through)?

The code I copied this from (V8DOMWrapper::setJSWrapperForActiveDOMObject) didn't have it.  It doesn't make a difference either way, since it'll fall through to the default case which breaks, but I added it anyways.

[Dimitri Glazkov (Google)]

> > Shouldn't there be a break right after line 221 (or else something that says
> > fall through)?
> 
> The code I copied this from (V8DOMWrapper::setJSWrapperForActiveDOMObject)
> didn't have it.  It doesn't make a difference either way, since it'll fall
> through to the default case which breaks, but I added it anyways.

You did? :)

[John Abd-El-Malek]

Created attachment 33990 [details]
Really update the patch

[David Levin]

Comment on attachment 33990 [details]
Really update the patch

Thanks John.

[Adam Barth]

John, do you mean to have this bug assigned to you?  In this state, it won't be committed out of the commit queue.  If you'd like me to commit it for you, you can reset the assignee to the default and nominate it landing by setting the commit-queue flag to "?"

[John Abd-El-Malek]

(In reply to comment #14)
> John, do you mean to have this bug assigned to you?  In this state, it won't be
> committed out of the commit queue.  If you'd like me to commit it for you, you
> can reset the assignee to the default and nominate it landing by setting the
> commit-queue flag to "?"

oops, thanks for alerting me, it doesn't have to be assigned to me anymore.  Don't see the commit-queue flag though.

[Adam Barth]

Comment on attachment 33990 [details]
Really update the patch

Clearing review flag on attachment: 33990

Sending        WebCore/ChangeLog
Sending        WebCore/bindings/v8/V8DOMWrapper.cpp
Sending        WebCore/bindings/v8/V8Proxy.cpp
Sending        WebCore/bindings/v8/WorkerContextExecutionProxy.cpp
Transmitting file data ....
Committed revision 46748.
http://trac.webkit.org/changeset/46748

[Adam Barth]

All reviewed patches have been landed.  Closing bug.