WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
177375
WeakPtrFactory should populate m_ref lazily.
https://bugs.webkit.org/show_bug.cgi?id=177375
Summary
WeakPtrFactory should populate m_ref lazily.
zalan
Reported
2017-09-22 12:08:18 PDT
-the first time createWeakPtr is called.
Attachments
Patch
(99.50 KB, patch)
2017-09-22 12:33 PDT
,
zalan
no flags
Details
Formatted Diff
Diff
Patch
(99.51 KB, patch)
2017-09-22 12:41 PDT
,
zalan
no flags
Details
Formatted Diff
Diff
Patch
(100.19 KB, patch)
2017-09-22 12:57 PDT
,
zalan
no flags
Details
Formatted Diff
Diff
Patch
(100.98 KB, patch)
2017-09-22 16:43 PDT
,
zalan
no flags
Details
Formatted Diff
Diff
Patch
(112.60 KB, patch)
2017-09-22 19:42 PDT
,
zalan
no flags
Details
Formatted Diff
Diff
Patch
(112.60 KB, patch)
2017-09-22 20:58 PDT
,
zalan
no flags
Details
Formatted Diff
Diff
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2017-09-22 12:31:31 PDT
<
rdar://problem/34600468
>
zalan
Comment 2
2017-09-22 12:33:17 PDT
Created
attachment 321579
[details]
Patch
zalan
Comment 3
2017-09-22 12:41:52 PDT
Created
attachment 321580
[details]
Patch
zalan
Comment 4
2017-09-22 12:57:46 PDT
Created
attachment 321584
[details]
Patch
Geoffrey Garen
Comment 5
2017-09-22 13:00:36 PDT
Comment on
attachment 321584
[details]
Patch r=me when it builds
zalan
Comment 6
2017-09-22 16:43:39 PDT
Created
attachment 321603
[details]
Patch
zalan
Comment 7
2017-09-22 19:42:22 PDT
Created
attachment 321612
[details]
Patch
WebKit Commit Bot
Comment 8
2017-09-22 20:56:34 PDT
Comment on
attachment 321612
[details]
Patch Rejecting
attachment 321612
[details]
from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 321612, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in Source/WTF/ChangeLog contains OOPS!. Full output:
http://webkit-queues.webkit.org/results/4633838
zalan
Comment 9
2017-09-22 20:58:47 PDT
Created
attachment 321614
[details]
Patch
WebKit Commit Bot
Comment 10
2017-09-22 21:38:50 PDT
Comment on
attachment 321614
[details]
Patch Clearing flags on attachment: 321614 Committed
r222422
: <
http://trac.webkit.org/changeset/222422
>
WebKit Commit Bot
Comment 11
2017-09-22 21:38:52 PDT
All reviewed patches have been landed. Closing bug.
Darin Adler
Comment 12
2017-09-23 14:47:51 PDT
Comment on
attachment 321614
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=321614&action=review
Thrilled to see this change! I had a very old patch in my git repository trying to do the same thing, but I never got it working 100%.
> Source/WTF/wtf/WeakPtr.h:56 > explicit WeakReference(T* ptr)
Can we change this constructor to take a reference instead of a pointer?
> Source/WTF/wtf/WeakPtr.h:-87 > -#ifndef NDEBUG > - ThreadIdentifier m_boundThread; > -#endif
What’s the rationale for removing the thread checking? Do we think that clients are now more sophisticated now and don’t need the help catching incorrect uses any longer? Or was it too hard to re-implement this? Or some other reason?
Geoffrey Garen
Comment 13
2017-09-25 18:16:04 PDT
> What’s the rationale for removing the thread checking? Do we think that > clients are now more sophisticated now and don’t need the help catching > incorrect uses any longer? Or was it too hard to re-implement this? Or some > other reason?
Too hard to reimplement: We (validly) use pointers on multiple threads, and one of the first pointers we changed into a weak pointer fired this assertion. You could imagine retaining this assertion as the default, with an escape hatch for "this pointer can be used on multiple threads". When we tried that with RefPtr, we eventually had to give it up because too many things needed the escape hatch. I'm hoping WeakPtr will be as successful as RefPtr.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug