Bug 54415 – Refactor handles and weak pointers to become nicer and more automatic

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Oliver Hunt]

Refactor handles and weak pointers to become nicer and more automatic

[Oliver Hunt]

Created attachment 82373 [details]
Patch

[WebKit Review Bot]

Attachment 82373 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSClassRef.cpp',..." exit_code: 1

Source/WebKit2/WebProcess/Plugins/Netscape/NPJSObject.h:49:  The parameter name "objectMap" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/WebProcess/Plugins/Netscape/NPJSObject.h:49:  The parameter name "jsObject" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/WebProcess/Plugins/Netscape/NPJSObject.h:65:  The parameter name "jsObject" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 3 in 89 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Attachment 82373 [details] did not build on win:
Build output: http://queues.webkit.org/results/7905761

[Oliver Hunt]

Created attachment 82385 [details]
Patch

[Oliver Hunt]

Only change is windows build fix.

[WebKit Review Bot]

Attachment 82385 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSClassRef.cpp',..." exit_code: 1

Source/WebKit2/WebProcess/Plugins/Netscape/NPJSObject.h:49:  The parameter name "objectMap" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/WebProcess/Plugins/Netscape/NPJSObject.h:49:  The parameter name "jsObject" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/WebProcess/Plugins/Netscape/NPJSObject.h:65:  The parameter name "jsObject" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 3 in 89 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Attachment 82373 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7913656

[Oliver Hunt]

Created attachment 82390 [details]
Patch

[Oliver Hunt]

Now fix qt

[WebKit Review Bot]

Attachment 82390 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSClassRef.cpp',..." exit_code: 1

Source/WebKit2/WebProcess/Plugins/Netscape/NPJSObject.h:49:  The parameter name "objectMap" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/WebProcess/Plugins/Netscape/NPJSObject.h:49:  The parameter name "jsObject" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/WebProcess/Plugins/Netscape/NPJSObject.h:65:  The parameter name "jsObject" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 3 in 90 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

Comment on attachment 82390 [details]
Patch

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

> Source/JavaScriptCore/API/JSClassRef.cpp:150
> -    protoDefinition.finalize = clearReferenceToPrototype;
> +    protoDefinition.finalize = 0;

No need to initialize to 0 -- you can just remove this now.

> Source/JavaScriptCore/collector/handles/Handle.h:77
> +        // It is unsafe to use a handle after invalidating it.
> +        m_slot = 0;

I don't think this comment is true anymore. You can certainly use a handle with a NULL m_slot. You just can't dereference it.

> Source/JavaScriptCore/collector/handles/SentinelLinkedList.h:1
> +/*

This file should go into WTF.

> Source/JavaScriptCore/collector/handles/SinglyLinkedList.h:1
> +/*

This one too.

> Source/JavaScriptCore/runtime/WeakGCPtr.h:114
> +template <typename T> class WeakGCPtr : public WeakGCPtrBase<T, false>  {

I'm still not convinced that it's worth the complexity of having two kinds of weak pointer. I only see one client of WeakGCPtr vs LazyWeakGCPtr. Why not make all WeakGCPtr lazy?

[Early Warning System Bot]

Attachment 82390 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7909770

[Csaba Osztrogonác]

(In reply to comment #12)
> Attachment 82390 [details] did not build on qt:
> Build output: http://queues.webkit.org/results/7909770

I am working on fixing Qt build. Please don't commit the patch before I finish. Thanks in advance.

[Csaba Osztrogonác]

Created attachment 82429 [details]
Qt buildfix

I attached the Qt buildfix for https://bugs.webkit.org/attachment.cgi?id=82390
I didn't set r?, because it isn't buildable itself, but it works with the original patch.

[Csaba Osztrogonác]

(In reply to comment #12)
> Attachment 82390 [details] did not build on qt:
> Build output: http://queues.webkit.org/results/7909770

Unfortunately an incremental build problem hid the real problem.
I filed a new bug report to fix the build system: https://bugs.webkit.org/show_bug.cgi?id=54466

[Oliver Hunt]

(In reply to comment #15)
> (In reply to comment #12)
> > Attachment 82390 [details] [details] did not build on qt:
> > Build output: http://queues.webkit.org/results/7909770
> 
> Unfortunately an incremental build problem hid the real problem.
> I filed a new bug report to fix the build system: https://bugs.webkit.org/show_bug.cgi?id=54466

Thanks!

I had been blocked waiting for the project file fixes to get through the ews bot so hadn't seen those failures, thanks for getting the qt_runtime fixes done as i would have had a lot of difficulty with them :D

[Oliver Hunt]

Created attachment 82513 [details]
Patch

[Oliver Hunt]

Don't review this, i'm just running it by the EWS bots as i had to make a bunch of project file changes.

[Geoffrey Garen]

I think it may be possible to remove the complexity of HandleBase+HandleConverter, and just give all handles get(), ->(), and *().

If all handles have get(), ->(), and *(), it becomes syntactically possible to get(), ->(), or *() an Unknown, which is a little unfortunate, but you can't actually do anything dangerous afterwards, since Unknown provides no API. No functions accept Unknowns as arguments, and Unknown has no data members or functions.

So, this is a compiler error since you can't assign Unknown to JSObject:

JSObject* object = handle.get();

This is a compiler error since Unknown doesn't have getOwnPropertySlot:

JSValue v = handle->getOwnPropertySlot();

etc.

[Early Warning System Bot]

Attachment 82513 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7918058

[Oliver Hunt]

Comment on attachment 82513 [details]
Patch

Already reviewed by gavin and geoff, was just waiting for build outpuit.  90% sure everything is good in what i've landed.

Committed r78634

[WebKit Review Bot]

http://trac.webkit.org/changeset/78634 might have broken EFL Linux Release (Build)