Bug 188938 - Make IDBCursor::m_request a WeakPtr
Summary: Make IDBCursor::m_request a WeakPtr
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-08-24 17:23 PDT by youenn fablet
Modified: 2018-08-26 19:45 PDT (History)
7 users (show)

See Also:


Attachments
Patch (4.71 KB, patch)
2018-08-24 17:26 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2018-08-24 17:23:49 PDT
This will ensure we will not try to use a request pointer that is no longer valid
Comment 1 youenn fablet 2018-08-24 17:26:50 PDT
Created attachment 348060 [details]
Patch
Comment 2 Alex Christensen 2018-08-24 21:17:06 PDT
Comment on attachment 348060 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:178
> +    if (!m_request)
> +        return Exception { InvalidStateError };

Can this be reached?  If so it should have a test.  If not, shouldn't it be an assert?
Comment 3 youenn fablet 2018-08-25 10:55:47 PDT
> > Source/WebCore/Modules/indexeddb/IDBCursor.cpp:178
> > +    if (!m_request)
> > +        return Exception { InvalidStateError };
> 
> Can this be reached?  If so it should have a test.  If not, shouldn't it be
> an assert?

This check is happening for advance and continue so if it is reachable for them, it makes sense to be reachable for continuePrimaryKey.

From my reading of the code, JSIDBCursor is protecting its JSIDBRequest.
The case where this error case might happen might be something like:
- IDBCursor is created with a request
- The request is never exposed to scripts so its JSIDBRequest does not exist.
- The request gets destroyed
- The cursor is asked to advance.

I can have a further look as a follow-up, I believe all 3 functions should be treated the same there, ie assert or exit early.
Comment 4 WebKit Commit Bot 2018-08-25 11:22:20 PDT
Comment on attachment 348060 [details]
Patch

Rejecting attachment 348060 [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-02', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 348060, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Logging in as commit-queue@webkit.org...
Fetching: https://bugs.webkit.org/attachment.cgi?id=348060&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=188938&ctype=xml&excludefield=attachmentdata
Processing 1 patch from 1 bug.
Updating working directory
Processing patch 348060 from bug 188938.
Fetching: https://bugs.webkit.org/attachment.cgi?id=348060
Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	Source/WebCore/ChangeLog
	M	Source/WebCore/Modules/indexeddb/IDBCursor.cpp
	M	Source/WebCore/Modules/indexeddb/IDBCursor.h
	M	Source/WebCore/Modules/indexeddb/IDBRequest.h

ERROR from SVN:
A repository hook failed: Commit blocked by pre-commit hook (exit code 1) with output:
Commits are currently disabled while we update infrastructure.
W: 7c4294d639c399bd273b69e1ccaa229dd9e07e24 and refs/remotes/origin/master differ, using rebase:
:040000 040000 ce40709dcfbc7fb25b1e5ed0ad3de8ebb0ec8a52 162abbfb909fd78b90c6722de4a4368f8d472b8a M	Source
Current branch master is up to date.
ERROR: Not all changes have been committed into SVN, however the committed
ones (if any) seem to be successfully integrated into the working tree.
Please see the above messages for details.


Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	Source/WebCore/ChangeLog
	M	Source/WebCore/Modules/indexeddb/IDBCursor.cpp
	M	Source/WebCore/Modules/indexeddb/IDBCursor.h
	M	Source/WebCore/Modules/indexeddb/IDBRequest.h

ERROR from SVN:
A repository hook failed: Commit blocked by pre-commit hook (exit code 1) with output:
Commits are currently disabled while we update infrastructure.
W: 7c4294d639c399bd273b69e1ccaa229dd9e07e24 and refs/remotes/origin/master differ, using rebase:
:040000 040000 ce40709dcfbc7fb25b1e5ed0ad3de8ebb0ec8a52 162abbfb909fd78b90c6722de4a4368f8d472b8a M	Source
Current branch master is up to date.
ERROR: Not all changes have been committed into SVN, however the committed
ones (if any) seem to be successfully integrated into the working tree.
Please see the above messages for details.


Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit
Updating OpenSource
Current branch master is up to date.

Full output: https://webkit-queues.webkit.org/results/8982502
Comment 5 WebKit Commit Bot 2018-08-26 19:44:24 PDT
Comment on attachment 348060 [details]
Patch

Clearing flags on attachment: 348060

Committed r235345: <https://trac.webkit.org/changeset/235345>
Comment 6 WebKit Commit Bot 2018-08-26 19:44:26 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Radar WebKit Bug Importer 2018-08-26 19:45:18 PDT
<rdar://problem/43740265>