Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2631153002:
Do not initiate fetch() on a destroyed FetchManager. (Closed)
DescriptionDo not initiate fetch() on a detached FetchManager.
R=
BUG=681378
Review-Url: https://codereview.chromium.org/2631153002
Cr-Commit-Position: refs/heads/master@{#443897}
Committed: https://chromium.googlesource.com/chromium/src/+/e7c73f3042d444e39255acdd2cb1b50a8c953836
Patch Set 1 #
Messages
Total messages: 21 (10 generated)
The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
sigbjornf@opera.com changed reviewers: + haraken@chromium.org, yhirano@chromium.org
please take a look. i've not been able to reproduce or synthesize a test for this detached use, but post-contextDestroyed() use of FetchManager does seem possible.
Description was changed from ========== Do not initiate fetch() on a destroyed FetchManager. R= BUG=681378 ========== to ========== Do not initiate fetch() on a detached FetchManager. R= BUG=681378 ==========
On 2017/01/16 11:08:34, sof wrote: > please take a look. > > i've not been able to reproduce or synthesize a test for this detached use, but > post-contextDestroyed() use of FetchManager does seem possible. Sorry I didn't have time to fix this... Maybe would it be an option to insert CHECK(m_fetchManager->getExecutionContext()) instead? Or is it too clear to fail and we won't get any useful information from it?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/01/16 11:17:07, haraken wrote: > On 2017/01/16 11:08:34, sof wrote: > > please take a look. > > > > i've not been able to reproduce or synthesize a test for this detached use, > but > > post-contextDestroyed() use of FetchManager does seem possible. > > Sorry I didn't have time to fix this... > > Maybe would it be an option to insert > CHECK(m_fetchManager->getExecutionContext()) instead? Or is it too clear to fail > and we won't get any useful information from it? That will turn a null-deref into a CHECK() failure; i don't think it'll bring us closer to determining how this can come about. The question here is if |contextDestroyed()| can be delivered by a Document before |contextIsValid()| on the ScriptState turns |false|. I think that's possible via DocumentLoader::prepareForCommit().
On 2017/01/16 12:18:13, sof wrote: > On 2017/01/16 11:17:07, haraken wrote: > > On 2017/01/16 11:08:34, sof wrote: > > > please take a look. > > > > > > i've not been able to reproduce or synthesize a test for this detached use, > > but > > > post-contextDestroyed() use of FetchManager does seem possible. > > > > Sorry I didn't have time to fix this... > > > > Maybe would it be an option to insert > > CHECK(m_fetchManager->getExecutionContext()) instead? Or is it too clear to > fail > > and we won't get any useful information from it? > > That will turn a null-deref into a CHECK() failure; i don't think it'll bring us > closer to determining how this can come about. > > The question here is if |contextDestroyed()| can be delivered by a Document > before |contextIsValid()| on the ScriptState turns |false|. I think that's > possible via DocumentLoader::prepareForCommit(). Ah... That can happen when we navigate from an initial empty document to a same-origin document. It means that contextIsValid() and getExecutionContext() are different checks, which we haven't assumed in the code base. Hmm... LGTM.
Is it possible that getExecutionContext() returns true but contextIsValid returns false?
On 2017/01/16 12:43:42, yhirano wrote: > Is it possible that getExecutionContext() returns true but contextIsValid > returns false? That is a good question, see https://bugs.chromium.org/p/chromium/issues/detail?id=678926#c5
haraken@chromium.org changed reviewers: + dcheng@chromium.org
On 2017/01/16 12:47:23, sof wrote: > On 2017/01/16 12:43:42, yhirano wrote: > > Is it possible that getExecutionContext() returns true but contextIsValid > > returns false? > > That is a good question, see > https://bugs.chromium.org/p/chromium/issues/detail?id=678926#c5 1) contextIsValid() : false, getExecutionContext() : nullptr => possible 2) contextIsValid() : false, getExecutionContext() : a valid context => not possible (as far as I understand) 3) contextIsValid() : true, getExecutionContext() : nullptr => possible (only when we navigate from an initial empty document to a same-origin document) 4) contextIsValid() : true, getExecutionContext() : valid context => possible 3) is really confusing and I want to think about a way to make the API less error-prone.
On 2017/01/16 12:51:25, haraken wrote: > On 2017/01/16 12:47:23, sof wrote: > > On 2017/01/16 12:43:42, yhirano wrote: > > > Is it possible that getExecutionContext() returns true but contextIsValid > > > returns false? > > > > That is a good question, see > > https://bugs.chromium.org/p/chromium/issues/detail?id=678926#c5 > > 1) contextIsValid() : false, getExecutionContext() : nullptr => possible > 2) contextIsValid() : false, getExecutionContext() : a valid context => not > possible (as far as I understand) > 3) contextIsValid() : true, getExecutionContext() : nullptr => possible (only > when we navigate from an initial empty document to a same-origin document) > 4) contextIsValid() : true, getExecutionContext() : valid context => possible > > 3) is really confusing and I want to think about a way to make the API less > error-prone. OK, LGTM, but I really want tests. Also, I'd like the binding layer handle these cases as I said at https://codereview.chromium.org/1897783002/#msg10.
On 2017/01/16 13:01:52, yhirano wrote: > On 2017/01/16 12:51:25, haraken wrote: > > On 2017/01/16 12:47:23, sof wrote: > > > On 2017/01/16 12:43:42, yhirano wrote: > > > > Is it possible that getExecutionContext() returns true but contextIsValid > > > > returns false? > > > > > > That is a good question, see > > > https://bugs.chromium.org/p/chromium/issues/detail?id=678926#c5 > > > > 1) contextIsValid() : false, getExecutionContext() : nullptr => possible > > 2) contextIsValid() : false, getExecutionContext() : a valid context => not > > possible (as far as I understand) > > 3) contextIsValid() : true, getExecutionContext() : nullptr => possible (only > > when we navigate from an initial empty document to a same-origin document) > > 4) contextIsValid() : true, getExecutionContext() : valid context => possible > > > > 3) is really confusing and I want to think about a way to make the API less > > error-prone. > > OK, LGTM, but I really want tests. Also, I'd like the binding layer handle these > cases as I said at https://codereview.chromium.org/1897783002/#msg10. Noted; tests are proving elusive, landing as-is.
The CQ bit was checked by sigbjornf@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 1, "attempt_start_ts": 1484579893616480, "parent_rev":
"919511bd14650ee9b2dde04116861f5c52db30c5", "commit_rev":
"e7c73f3042d444e39255acdd2cb1b50a8c953836"}
Message was sent while issue was closed.
Description was changed from ========== Do not initiate fetch() on a detached FetchManager. R= BUG=681378 ========== to ========== Do not initiate fetch() on a detached FetchManager. R= BUG=681378 Review-Url: https://codereview.chromium.org/2631153002 Cr-Commit-Position: refs/heads/master@{#443897} Committed: https://chromium.googlesource.com/chromium/src/+/e7c73f3042d444e39255acdd2cb1... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/e7c73f3042d444e39255acdd2cb1... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
