Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2714643008:
NetworkStateNotifier: also remove NetworkStateObserver in finalizer too (Closed)
DescriptionNetworkStateNotifier: also remove NetworkStateObserver in finalizer too
I was wrong that contextDestroy is not always called before the
document gets destructed.
Make sure we remove NetworkStateNotifier in finalizer too to avoid crash.
BUG=695783
TEST=See the bug for repro
Review-Url: https://codereview.chromium.org/2714643008
Cr-Commit-Position: refs/heads/master@{#452870}
Committed: https://chromium.googlesource.com/chromium/src/+/ef33cbc2c4e56e2aa5c1503b7cbaf0b826c3affe
Patch Set 1 #
Total comments: 7
Patch Set 2 : . #
Total comments: 2
Patch Set 3 : add comment + another method #Messages
Total messages: 36 (16 generated)
The CQ bit was checked by kinuko@chromium.org 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...
Description was changed from ========== NetworkStateNotifier: remove NetworkStateObserver in pre-finalizer too I was wrong that contextDestroy is not always called before the document gets destructed. Make sure we remove NetworkStateNotifier in pre-finalizer too to avoid crash. BUG=695783 TEST=See the bug for repro ========== to ========== NetworkStateNotifier: also remove NetworkStateObserver in pre-finalizer too I was wrong that contextDestroy is not always called before the document gets destructed. Make sure we remove NetworkStateNotifier in pre-finalizer too to avoid crash. BUG=695783 TEST=See the bug for repro ==========
kinuko@chromium.org changed reviewers: + dcheng@chromium.org, jkarlin@chromium.org
So looks like I was wrong about Document / NetworkStateObserver destruction... could you review?
https://codereview.chromium.org/2714643008/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2714643008/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Document.cpp:413: USING_PRE_FINALIZER(Document::NetworkStateObserver, dispose); Why PRE_FINALIZER instead of normal destructor? https://codereview.chromium.org/2714643008/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Document.cpp:447: bool detached = false; Forgot the m_. nit: Perhaps m_isObserving?
The CQ bit was checked by kinuko@chromium.org to run a CQ dry run
kinuko@chromium.org changed reviewers: + oilpan-reviews@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #2 (id:20001) has been deleted
Description was changed from ========== NetworkStateNotifier: also remove NetworkStateObserver in pre-finalizer too I was wrong that contextDestroy is not always called before the document gets destructed. Make sure we remove NetworkStateNotifier in pre-finalizer too to avoid crash. BUG=695783 TEST=See the bug for repro ========== to ========== NetworkStateNotifier: also remove NetworkStateObserver in finalizer too I was wrong that contextDestroy is not always called before the document gets destructed. Make sure we remove NetworkStateNotifier in finalizer too to avoid crash. BUG=695783 TEST=See the bug for repro ==========
The CQ bit was checked by kinuko@chromium.org 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...
Thanks! +oilpan-reviews@ too https://codereview.chromium.org/2714643008/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2714643008/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Document.cpp:413: USING_PRE_FINALIZER(Document::NetworkStateObserver, dispose); On 2017/02/24 13:13:51, jkarlin wrote: > Why PRE_FINALIZER instead of normal destructor? Removing observer touches on-heap object, so I think it needs to be done in pre-finalizer or in eagerly finalized way. Switched to use EAGERLY_FINALIZE to do it in destructor (I slightly prefer pre-finalizer but either looks ok here) https://codereview.chromium.org/2714643008/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Document.cpp:447: bool detached = false; On 2017/02/24 13:13:51, jkarlin wrote: > Forgot the m_. > > nit: Perhaps m_isObserving? Removed this flag in favor of checking getExecutionContext().
https://codereview.chromium.org/2714643008/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2714643008/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Document.cpp:413: USING_PRE_FINALIZER(Document::NetworkStateObserver, dispose); On 2017/02/24 13:56:25, kinuko wrote: > On 2017/02/24 13:13:51, jkarlin wrote: > > Why PRE_FINALIZER instead of normal destructor? > > Removing observer touches on-heap object, so I think it needs to be done in > pre-finalizer or in eagerly finalized way. Switched to use EAGERLY_FINALIZE to > do it in destructor (I slightly prefer pre-finalizer but either looks ok here) What object does it touch that's on the oilpan heap?
sigbjornf@opera.com changed reviewers: + sigbjornf@opera.com
lgtm from my side, a case of Document::shutdown() not being called. I wonder if we can avoid having NetworkStateNotifier keep unsafe raw observer pointers (and turn them into weak references instead), but that's for later, if feasible. https://codereview.chromium.org/2714643008/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2714643008/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:423: if (getExecutionContext()) It's safe to do this eagerly, but how about introducing void unregisterAsObserver(ExecutionContext* context) { if (!context) return; ... } or something similarly named, which we then call from both the dtor and contextDestroyed()? It's just shifting lines of code around, but calling contextDestroyed() is preferably left (only) for the LifecycleNotifier to do.
Thanks! I agree that we should avoid storing unsafe raw observer pointers, will try to see if I can make a follow-up patch. https://codereview.chromium.org/2714643008/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2714643008/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Document.cpp:413: USING_PRE_FINALIZER(Document::NetworkStateObserver, dispose); On 2017/02/24 14:20:06, jkarlin wrote: > On 2017/02/24 13:56:25, kinuko wrote: > > On 2017/02/24 13:13:51, jkarlin wrote: > > > Why PRE_FINALIZER instead of normal destructor? > > > > Removing observer touches on-heap object, so I think it needs to be done in > > pre-finalizer or in eagerly finalized way. Switched to use EAGERLY_FINALIZE > to > > do it in destructor (I slightly prefer pre-finalizer but either looks ok here) > > What object does it touch that's on the oilpan heap? m_executionContext in contextLifecycleObserver, and TaskRunnerHelper::get() derefs it (hm, we could also keep the task runner around). Also dtor could be called much later without EAGERLY_FINALIZE, which seems dangerous if we don't unregister our callbacks earlier. https://codereview.chromium.org/2714643008/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2714643008/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:423: if (getExecutionContext()) On 2017/02/24 14:21:59, sof wrote: > It's safe to do this eagerly, but how about introducing > > void unregisterAsObserver(ExecutionContext* context) { > if (!context) > return; > ... > } > > or something similarly named, which we then call from both the dtor and > contextDestroyed()? > > It's just shifting lines of code around, but calling contextDestroyed() is > preferably left (only) for the LifecycleNotifier to do. Sounds good, done.
lgtm https://codereview.chromium.org/2714643008/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2714643008/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Document.cpp:413: USING_PRE_FINALIZER(Document::NetworkStateObserver, dispose); On 2017/02/24 14:43:16, kinuko wrote: > On 2017/02/24 14:20:06, jkarlin wrote: > > On 2017/02/24 13:56:25, kinuko wrote: > > > On 2017/02/24 13:13:51, jkarlin wrote: > > > > Why PRE_FINALIZER instead of normal destructor? > > > > > > Removing observer touches on-heap object, so I think it needs to be done in > > > pre-finalizer or in eagerly finalized way. Switched to use EAGERLY_FINALIZE > > to > > > do it in destructor (I slightly prefer pre-finalizer but either looks ok > here) > > > > What object does it touch that's on the oilpan heap? > > m_executionContext in contextLifecycleObserver, and TaskRunnerHelper::get() Ah yes, thanks!
The CQ bit was checked by jkarlin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sigbjornf@opera.com Link to the patchset: https://codereview.chromium.org/2714643008/#ps60001 (title: "add comment + another method")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I hit the commit button due to the high crash rate.
Can you help me understand why Document::shutdown() is not called? That seems like a bug.
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487953507935720,
"parent_rev": "079cef1cc5eea119425834494676994825ae9fae", "commit_rev":
"ef33cbc2c4e56e2aa5c1503b7cbaf0b826c3affe"}
Message was sent while issue was closed.
Description was changed from ========== NetworkStateNotifier: also remove NetworkStateObserver in finalizer too I was wrong that contextDestroy is not always called before the document gets destructed. Make sure we remove NetworkStateNotifier in finalizer too to avoid crash. BUG=695783 TEST=See the bug for repro ========== to ========== NetworkStateNotifier: also remove NetworkStateObserver in finalizer too I was wrong that contextDestroy is not always called before the document gets destructed. Make sure we remove NetworkStateNotifier in finalizer too to avoid crash. BUG=695783 TEST=See the bug for repro Review-Url: https://codereview.chromium.org/2714643008 Cr-Commit-Position: refs/heads/master@{#452870} Committed: https://chromium.googlesource.com/chromium/src/+/ef33cbc2c4e56e2aa5c1503b7cba... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/chromium/src/+/ef33cbc2c4e56e2aa5c1503b7cba...
Message was sent while issue was closed.
On 2017/02/24 17:57:12, dcheng wrote: > Can you help me understand why Document::shutdown() is not called? That seems > like a bug. I haven't looked at the repro, but I assume it is due to https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Docum...
Message was sent while issue was closed.
On 2017/02/24 18:22:55, sof wrote: > On 2017/02/24 17:57:12, dcheng wrote: > > Can you help me understand why Document::shutdown() is not called? That seems > > like a bug. > > I haven't looked at the repro, but I assume it is due to > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Docum... Maybe the Document shouldn't be registering a network observer in that case though?
Message was sent while issue was closed.
On 2017/02/24 18:26:46, dcheng wrote: > On 2017/02/24 18:22:55, sof wrote: > > On 2017/02/24 17:57:12, dcheng wrote: > > > Can you help me understand why Document::shutdown() is not called? That > seems > > > like a bug. > > > > I haven't looked at the repro, but I assume it is due to > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Docum... > > Maybe the Document shouldn't be registering a network observer in that case > though? Restrict online/offline events ( https://html.spec.whatwg.org/multipage/browsers.html#navigator.online ) to "attached" Documents ?
Message was sent while issue was closed.
On 2017/02/24 18:41:52, sof wrote: > On 2017/02/24 18:26:46, dcheng wrote: > > On 2017/02/24 18:22:55, sof wrote: > > > On 2017/02/24 17:57:12, dcheng wrote: > > > > Can you help me understand why Document::shutdown() is not called? That > > seems > > > > like a bug. > > > > > > I haven't looked at the repro, but I assume it is due to > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Docum... > > > > Maybe the Document shouldn't be registering a network observer in that case > > though? > > Restrict online/offline events ( > https://html.spec.whatwg.org/multipage/browsers.html#navigator.online ) to > "attached" Documents ? TBh, I'm not entirely sure what the right behavior here is. I know we want to make it possible to fire events/run script in detached docs (or at least more so), but we also need to be mindful of what we enable. It does seem wrong that something that's a context lifecycle observer would ever need to eagerly finalize though.
Message was sent while issue was closed.
On 2017/02/24 19:24:13, dcheng wrote: > On 2017/02/24 18:41:52, sof wrote: > > On 2017/02/24 18:26:46, dcheng wrote: > > > On 2017/02/24 18:22:55, sof wrote: > > > > On 2017/02/24 17:57:12, dcheng wrote: > > > > > Can you help me understand why Document::shutdown() is not called? That > > > seems > > > > > like a bug. > > > > > > > > I haven't looked at the repro, but I assume it is due to > > > > > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Docum... > > > > > > Maybe the Document shouldn't be registering a network observer in that case > > > though? > > > > Restrict online/offline events ( > > https://html.spec.whatwg.org/multipage/browsers.html#navigator.online ) to > > "attached" Documents ? > > TBh, I'm not entirely sure what the right behavior here is. I know we want to > make it possible to fire events/run script in detached docs (or at least more > so), but we also need to be mindful of what we enable. It does seem wrong that > something that's a context lifecycle observer would ever need to eagerly > finalize though. Agree, it is causing bugs we can do without (XHR is another example.) Turning the tables and having Document be prefinalized, might work.
Message was sent while issue was closed.
On 2017/02/24 20:30:01, sof wrote: > On 2017/02/24 19:24:13, dcheng wrote: > > On 2017/02/24 18:41:52, sof wrote: > > > On 2017/02/24 18:26:46, dcheng wrote: > > > > On 2017/02/24 18:22:55, sof wrote: > > > > > On 2017/02/24 17:57:12, dcheng wrote: > > > > > > Can you help me understand why Document::shutdown() is not called? > That > > > > seems > > > > > > like a bug. > > > > > > > > > > I haven't looked at the repro, but I assume it is due to > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Docum... > > > > > > > > Maybe the Document shouldn't be registering a network observer in that > case > > > > though? > > > > > > Restrict online/offline events ( > > > https://html.spec.whatwg.org/multipage/browsers.html#navigator.online ) to > > > "attached" Documents ? > > > > TBh, I'm not entirely sure what the right behavior here is. I know we want to > > make it possible to fire events/run script in detached docs (or at least more > > so), but we also need to be mindful of what we enable. It does seem wrong that > > something that's a context lifecycle observer would ever need to eagerly > > finalize though. > > Agree, it is causing bugs we can do without (XHR is another example.) Turning > the tables and having Document be prefinalized, might work. Yeah, this CL is doing something weird... Document::shudown() is guaranteed to be called for documents that are attached to frames. Would it be possible to stop attaching a context lifecycle observer to an ExecutionContext that isn't attached to a frame (e.g., a document created via DOMImplementation)? Or will it break other things? (i.e., is there any actual scenario where we want to attach a context lifecycle observer to an ExecutionContext that isn't attached to a frame?)
Message was sent while issue was closed.
On 2017/02/25 00:43:11, haraken wrote: > On 2017/02/24 20:30:01, sof wrote: > > On 2017/02/24 19:24:13, dcheng wrote: > > > On 2017/02/24 18:41:52, sof wrote: > > > > On 2017/02/24 18:26:46, dcheng wrote: > > > > > On 2017/02/24 18:22:55, sof wrote: > > > > > > On 2017/02/24 17:57:12, dcheng wrote: > > > > > > > Can you help me understand why Document::shutdown() is not called? > > That > > > > > seems > > > > > > > like a bug. > > > > > > > > > > > > I haven't looked at the repro, but I assume it is due to > > > > > > > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Docum... > > > > > > > > > > Maybe the Document shouldn't be registering a network observer in that > > case > > > > > though? > > > > > > > > Restrict online/offline events ( > > > > https://html.spec.whatwg.org/multipage/browsers.html#navigator.online ) to > > > > "attached" Documents ? > > > > > > TBh, I'm not entirely sure what the right behavior here is. I know we want > to > > > make it possible to fire events/run script in detached docs (or at least > more > > > so), but we also need to be mindful of what we enable. It does seem wrong > that > > > something that's a context lifecycle observer would ever need to eagerly > > > finalize though. > > > > Agree, it is causing bugs we can do without (XHR is another example.) Turning > > the tables and having Document be prefinalized, might work. > > Yeah, this CL is doing something weird... > > Document::shudown() is guaranteed to be called for documents that are attached > to frames. Would it be possible to stop attaching a context lifecycle observer > to an ExecutionContext that isn't attached to a frame (e.g., a document created > via DOMImplementation)? Or will it break other things? (i.e., is there any > actual scenario where we want to attach a context lifecycle observer to an > ExecutionContext that isn't attached to a frame?) I agree that what this CL does is very unfortunate / undesirable. I wanted to stop registering the observer when the document is not attached but wasn't sure about the right behavior either, and needed a fix to stop crashes first ¯\_(ツ)_/¯ I could try making the follow-up patch, or I wonder if Document could just make sure it always advance the lifecycle to call contextDestroyed in prefinalizer if it hasn't? (By the way if you talk about ExecutionContext but not Document let me remind that ExecutionContext isn't / shouldn't be necessarily associated with Frames)
Message was sent while issue was closed.
This CL LGTM as a short-term fix. > > Yeah, this CL is doing something weird... > > > > Document::shudown() is guaranteed to be called for documents that are attached > > to frames. Would it be possible to stop attaching a context lifecycle observer > > to an ExecutionContext that isn't attached to a frame (e.g., a document > created > > via DOMImplementation)? Or will it break other things? (i.e., is there any > > actual scenario where we want to attach a context lifecycle observer to an > > ExecutionContext that isn't attached to a frame?) > > I agree that what this CL does is very unfortunate / undesirable. I wanted to > stop registering the observer when the document is not attached but wasn't sure > about the right behavior either, and needed a fix to stop crashes first > ¯\_(ツ)_/¯ I could try making the follow-up patch, or I wonder if Document could > just make sure it always advance the lifecycle to call contextDestroyed in > prefinalizer if it hasn't? Before trying to introducing a pre-finalizer to Document, I might want to see if we can stop attaching a context lifecycle observer to a Document that is not attached to a frame. (However, I'm not sure how easy the change will be.) A pre-finalizer is a bit weird since 1) the timing is GC-dependent (if you do something complicated in contextDestroyed, you may end up with exposing a GC behavior to user scripts), 2) the pre-finalizer is called during a GC (so you're not allowed to do something complicated in contextDestroyed) and 3) the pre-finalizer doesn't get called until Document becomes unreachable (if you're clearing a pointer to the document in contextDestroyed, it will leak the document). > (By the way if you talk about ExecutionContext but not Document let me remind > that ExecutionContext isn't / shouldn't be necessarily associated with Frames) Ah, yes, I meant Document. Documents that are not attached to frames don't call notifyContextDestroyed().
Message was sent while issue was closed.
On 2017/02/25 02:59:01, haraken wrote: > This CL LGTM as a short-term fix. > > > > Yeah, this CL is doing something weird... > > > > > > Document::shudown() is guaranteed to be called for documents that are > attached > > > to frames. Would it be possible to stop attaching a context lifecycle > observer > > > to an ExecutionContext that isn't attached to a frame (e.g., a document > > created > > > via DOMImplementation)? Or will it break other things? (i.e., is there any > > > actual scenario where we want to attach a context lifecycle observer to an > > > ExecutionContext that isn't attached to a frame?) > > > > I agree that what this CL does is very unfortunate / undesirable. I wanted to > > stop registering the observer when the document is not attached but wasn't > sure > > about the right behavior either, and needed a fix to stop crashes first > > ¯\_(ツ)_/¯ I could try making the follow-up patch, or I wonder if Document > could > > just make sure it always advance the lifecycle to call contextDestroyed in > > prefinalizer if it hasn't? > > Before trying to introducing a pre-finalizer to Document, I might want to see if > we can stop attaching a context lifecycle observer to a Document that is not > attached to a frame. (However, I'm not sure how easy the change will be.) Do you mean by having an assert or something and see what'll get broken? I'm not entirely sure if we could (or should) convert all the observers, but for this particular one it's probably likely ok not to attach when it's not attached to frames. I'll create a follow-up patch and send a review- > A pre-finalizer is a bit weird since 1) the timing is GC-dependent (if you do > something complicated in contextDestroyed, you may end up with exposing a GC > behavior to user scripts), 2) the pre-finalizer is called during a GC (so you're > not allowed to do something complicated in contextDestroyed) and 3) the > pre-finalizer doesn't get called until Document becomes unreachable (if you're > clearing a pointer to the document in contextDestroyed, it will leak the > document). > > > (By the way if you talk about ExecutionContext but not Document let me remind > > that ExecutionContext isn't / shouldn't be necessarily associated with Frames) > > Ah, yes, I meant Document. Documents that are not attached to frames don't call > notifyContextDestroyed(). |
