Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2748603002:
Add a lifecycle state machine for Page (Closed)
DescriptionAdd a lifecycle state machine for Frame
This CL adds a linear state machine to the Frame to ensure that it
follows an orderly progression through its state machine. The plan is to
use this state machine to make an isAttached() method which can check
this state, and later make page(), client(), host() etc return
references that DCHECK on the correct state. Also removed m_isDetaching
and replaced it with checks on the lifecycle.
Based on abarth's CL for adding a state machine for Document in:
https://chromiumcodereview.appspot.com/25036004
BUG=700783
Review-Url: https://codereview.chromium.org/2748603002
Cr-Commit-Position: refs/heads/master@{#459710}
Committed: https://chromium.googlesource.com/chromium/src/+/382a2483590ea3a82082d78c36b1f10a65ac7501
Patch Set 1 #Patch Set 2 : Added missing files #
Total comments: 8
Patch Set 3 : Moved PageLifecycle to FrameLifecycle #
Total comments: 13
Patch Set 4 : Review feedback #Patch Set 5 : Allowed movement from Detached back to the same state. #
Total comments: 7
Patch Set 6 : Review feedback #Patch Set 7 : Added check to detach() call in Page #
Total comments: 2
Patch Set 8 : Added TODO to fix temp if statement #Patch Set 9 : Rebase #Messages
Total messages: 61 (44 generated)
The CQ bit was checked by sashab@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...
The CQ bit was checked by sashab@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 ========== Add a lifecycle state machine for Page (WIP) This CL adds a linear state machine to the Page to ensure that it follows an orderly progression through its state machine. The plan is to use this state machine to make an isAttached() method which can check this state, and later make page(), client(), host() etc return references that DCHECK on the correct state. BUG=TODO ========== to ========== Add a lifecycle state machine for Page (WIP) This CL adds a linear state machine to the Page to ensure that it follows an orderly progression through its state machine. The plan is to use this state machine to make an isAttached() method which can check this state, and later make page(), client(), host() etc return references that DCHECK on the correct state. Based on abarth's CL for adding a state machine for Document in: https://chromiumcodereview.appspot.com/25036004 BUG=TODO ==========
sashab@chromium.org changed reviewers: + dcheng@chromium.org
Hi daniel, just playing around with ideas at the moment, have some questions for you on the CL. Will file a bug for this at some point too. Thanks! :) https://codereview.chromium.org/2748603002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/Page.cpp (right): https://codereview.chromium.org/2748603002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/Page.cpp:127: allPages().insert(this); Document also has an "Inactive" state after "Uninitialized" that gets set at the end of the constructor. Is that useful here? I didn't think it was so I left it out. https://codereview.chromium.org/2748603002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/Page.cpp:201: m_lifecycle.advanceTo(PageLifecycle::Attaching); Are Attaching/Attached and Detaching/Detached good names for describing these 2 states? And are these the right places to check them? From reading through the code these seem like the critical methods. https://codereview.chromium.org/2748603002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/Page.cpp:608: m_lifecycle.advanceTo(PageLifecycle::Disposed); Not sure how to check this, other than putting it here. https://codereview.chromium.org/2748603002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/PageLifecycle.cpp (right): https://codereview.chromium.org/2748603002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/PageLifecycle.cpp:14: DCHECK_GT(state, m_state); This is obviously not true - a Page can go from Detached to Attached again, am I correct? Will need to add more complex logic here.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Add a lifecycle state machine for Page (WIP) This CL adds a linear state machine to the Page to ensure that it follows an orderly progression through its state machine. The plan is to use this state machine to make an isAttached() method which can check this state, and later make page(), client(), host() etc return references that DCHECK on the correct state. Based on abarth's CL for adding a state machine for Document in: https://chromiumcodereview.appspot.com/25036004 BUG=TODO ========== to ========== Add a lifecycle state machine for Page (WIP) This CL adds a linear state machine to the Page to ensure that it follows an orderly progression through its state machine. The plan is to use this state machine to make an isAttached() method which can check this state, and later make page(), client(), host() etc return references that DCHECK on the correct state. Based on abarth's CL for adding a state machine for Document in: https://chromiumcodereview.appspot.com/25036004 BUG=700783 ==========
Ping dcheng, elliott feel free to comment too
Sorry for the slow review, I was OOO on Friday and Monday. My main question is: why Page instead of Frame? Page itself doesn't have too many members that go null when destroyed,so I'm uncertain of the utility of Page lifecycle. https://codereview.chromium.org/2748603002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/Page.cpp (right): https://codereview.chromium.org/2748603002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/Page.cpp:127: allPages().insert(this); On 2017/03/13 03:01:52, sashab wrote: > Document also has an "Inactive" state after "Uninitialized" that gets set at the > end of the constructor. Is that useful here? I didn't think it was so I left it > out. I think it's fine to leave it uninitialized here. https://codereview.chromium.org/2748603002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/Page.cpp:201: m_lifecycle.advanceTo(PageLifecycle::Attaching); On 2017/03/13 03:01:52, sashab wrote: > Are Attaching/Attached and Detaching/Detached good names for describing these 2 > states? And are these the right places to check them? From reading through the > code these seem like the critical methods. I'm not entirely sure we actually need to distinguish between the -ing and -ed forms, but it doesn't hurt to have them for now. https://codereview.chromium.org/2748603002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/Page.cpp:212: m_lifecycle.advanceTo(PageLifecycle::Detaching); I think this be moved to willBeDestroyed(), as this is called for each Document that's shutdown each time, while a Page is really only detached once we call WebView::close() https://codereview.chromium.org/2748603002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/PageLifecycle.cpp (right): https://codereview.chromium.org/2748603002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/PageLifecycle.cpp:14: DCHECK_GT(state, m_state); On 2017/03/13 03:01:52, sashab wrote: > This is obviously not true - a Page can go from Detached to Attached again, am I > correct? Will need to add more complex logic here. See my previous comments.
Yeah I think this is meant to be FrameLifecycle instead. We might want a PageLifecycle eventually to, but I can't quite imagine how that works yet.
The CQ bit was checked by sashab@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 ========== Add a lifecycle state machine for Page (WIP) This CL adds a linear state machine to the Page to ensure that it follows an orderly progression through its state machine. The plan is to use this state machine to make an isAttached() method which can check this state, and later make page(), client(), host() etc return references that DCHECK on the correct state. Based on abarth's CL for adding a state machine for Document in: https://chromiumcodereview.appspot.com/25036004 BUG=700783 ========== to ========== Add a lifecycle state machine for Frame (WIP) This CL adds a linear state machine to the Frame to ensure that it follows an orderly progression through its state machine. The plan is to use this state machine to make an isAttached() method which can check this state, and later make page(), client(), host() etc return references that DCHECK on the correct state. Also removed m_isDetaching and replaced it with checks on the lifecycle. Based on abarth's CL for adding a state machine for Document in: https://chromiumcodereview.appspot.com/25036004 BUG=700783 ==========
Thanks elliott & daniel - you are absolutely right, I meant for this to be FrameLifecycle :) Moved the lifecycle to Frame and had another go, ptal. Also removed m_isDetaching and replaced it with the lifecycle check, this seems to work locally, waiting for trybots to see if I've implemented it correctly. https://codereview.chromium.org/2748603002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/Frame.h (right): https://codereview.chromium.org/2748603002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/Frame.h:152: bool hasDetached() const { I renamed this to hasDetached() since it's really checking whether the frame is in Detaching state or later.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2748603002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/Frame.cpp (right): https://codereview.chromium.org/2748603002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/Frame.cpp:61: m_lifecycle.advanceTo(FrameLifecycle::Disposed); My personal opinion is this is unnecessary: accessing Frame after this point is a clear UaF, and will be detected in ASAN builds: Oilpan will mark the memory as poisoned after finalizing an object. https://codereview.chromium.org/2748603002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/Frame.cpp:74: m_lifecycle.advanceTo(FrameLifecycle::Detaching); I would suggest DCHECKing that we're already in Detaching here, as Frame is an abstract class and the subclasses should have already advanced it to detaching (which means that this advanceTo would probably DCHECK, given the DCHECK_GT in advanceTo). https://codereview.chromium.org/2748603002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/Frame.cpp:433: m_lifecycle.advanceTo(FrameLifecycle::Attached); I would probably just start the frame in Attached mode rather than having uninitialized. I'm not sure it's necessary to distinguish unless the logic here changes in the future. https://codereview.chromium.org/2748603002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/Frame.h (right): https://codereview.chromium.org/2748603002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/Frame.h:152: bool hasDetached() const { On 2017/03/16 01:01:04, sashab wrote: > I renamed this to hasDetached() since it's really checking whether the frame is > in Detaching state or later. I don't have a good name for this, TBH. hasDetached() kind of implies that the detach process is already done, which is not necessarily true. How about just calling this isAttached() and checking state == Attached? https://codereview.chromium.org/2748603002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameLifecycle.cpp (right): https://codereview.chromium.org/2748603002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameLifecycle.cpp:11: FrameLifecycle::~FrameLifecycle() {} This type can be trivially destroyed, so just omit the destructor. https://codereview.chromium.org/2748603002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right): https://codereview.chromium.org/2748603002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalFrame.cpp:462: m_lifecycle.advanceTo(FrameLifecycle::Detached); Hmm... this is a bit tricky. We have this in the base method too. I guess we should just do all the advancing in the subclasses; otherwise, this means we double-advance to certain states, which is a bit unusual.
The CQ bit was checked by sashab@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by sashab@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 dcheng, ptal https://codereview.chromium.org/2748603002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/Frame.cpp (right): https://codereview.chromium.org/2748603002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/Frame.cpp:61: m_lifecycle.advanceTo(FrameLifecycle::Disposed); On 2017/03/16 at 05:52:28, dcheng wrote: > My personal opinion is this is unnecessary: accessing Frame after this point is a clear UaF, and will be detected in ASAN builds: Oilpan will mark the memory as poisoned after finalizing an object. Agreed, removed this and the 'Disposed' state altogether. https://codereview.chromium.org/2748603002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/Frame.cpp:74: m_lifecycle.advanceTo(FrameLifecycle::Detaching); On 2017/03/16 at 05:52:28, dcheng wrote: > I would suggest DCHECKing that we're already in Detaching here, as Frame is an abstract class and the subclasses should have already advanced it to detaching (which means that this advanceTo would probably DCHECK, given the DCHECK_GT in advanceTo). Done. https://codereview.chromium.org/2748603002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/Frame.cpp:433: m_lifecycle.advanceTo(FrameLifecycle::Attached); On 2017/03/16 at 05:52:28, dcheng wrote: > I would probably just start the frame in Attached mode rather than having uninitialized. I'm not sure it's necessary to distinguish unless the logic here changes in the future. Ok, this is a good idea. Made this patch much simpler by removing the Uninitialized and Attaching stages; Frames now just start Attached. https://codereview.chromium.org/2748603002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/Frame.h (right): https://codereview.chromium.org/2748603002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/Frame.h:152: bool hasDetached() const { On 2017/03/16 at 05:52:28, dcheng wrote: > On 2017/03/16 01:01:04, sashab wrote: > > I renamed this to hasDetached() since it's really checking whether the frame is > > in Detaching state or later. > > I don't have a good name for this, TBH. hasDetached() kind of implies that the detach process is already done, which is not necessarily true. How about just calling this isAttached() and checking state == Attached? Nice! Done. https://codereview.chromium.org/2748603002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameLifecycle.cpp (right): https://codereview.chromium.org/2748603002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameLifecycle.cpp:11: FrameLifecycle::~FrameLifecycle() {} On 2017/03/16 at 05:52:28, dcheng wrote: > This type can be trivially destroyed, so just omit the destructor. Done. https://codereview.chromium.org/2748603002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right): https://codereview.chromium.org/2748603002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalFrame.cpp:462: m_lifecycle.advanceTo(FrameLifecycle::Detached); On 2017/03/16 at 05:52:29, dcheng wrote: > Hmm... this is a bit tricky. We have this in the base method too. I guess we should just do all the advancing in the subclasses; otherwise, this means we double-advance to certain states, which is a bit unusual. Good call; done, added DCHECKs in the Frame class, so you can't just call it directly from anywhere else (the DCHECK will fail).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sashab@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...
dcheng@ ptal, trybots should pass now. Needed to add logic to allow Detached state to be re-entered. https://codereview.chromium.org/2748603002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameLifecycle.cpp (right): https://codereview.chromium.org/2748603002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameLifecycle.cpp:14: switch (state) { dcheng@ -- think this is the best place for this logic? Wasn't sure whether to put this here or in the callers, eg add an if-check to only advance state in LocalFrame::detach() if we're not already in the Detaching state.
Description was changed from ========== Add a lifecycle state machine for Frame (WIP) This CL adds a linear state machine to the Frame to ensure that it follows an orderly progression through its state machine. The plan is to use this state machine to make an isAttached() method which can check this state, and later make page(), client(), host() etc return references that DCHECK on the correct state. Also removed m_isDetaching and replaced it with checks on the lifecycle. Based on abarth's CL for adding a state machine for Document in: https://chromiumcodereview.appspot.com/25036004 BUG=700783 ========== to ========== Add a lifecycle state machine for Frame This CL adds a linear state machine to the Frame to ensure that it follows an orderly progression through its state machine. The plan is to use this state machine to make an isAttached() method which can check this state, and later make page(), client(), host() etc return references that DCHECK on the correct state. Also removed m_isDetaching and replaced it with checks on the lifecycle. Based on abarth's CL for adding a state machine for Document in: https://chromiumcodereview.appspot.com/25036004 BUG=700783 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2748603002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/Frame.cpp (right): https://codereview.chromium.org/2748603002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/Frame.cpp:61: ASSERT(!m_owner); Shall we assert that the the lifecycle is in the detached state here? https://codereview.chromium.org/2748603002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/Frame.cpp:75: m_lifecycle.advanceTo(FrameLifecycle::Detaching); Instead of advancing to detaching here, can we assert that we are already in the detaching state? Both of the subclasses should have advanced it already. https://codereview.chromium.org/2748603002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameLifecycle.cpp (right): https://codereview.chromium.org/2748603002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameLifecycle.cpp:14: switch (state) { On 2017/03/22 03:05:28, sashab wrote: > dcheng@ -- think this is the best place for this logic? Wasn't sure whether to > put this here or in the callers, eg add an if-check to only advance state in > LocalFrame::detach() if we're not already in the Detaching state. This makes sense to me.
The CQ bit was checked by sashab@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sashab@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 daniel; ptal. Had to add a check in Page, see comment. https://codereview.chromium.org/2748603002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/Frame.cpp (right): https://codereview.chromium.org/2748603002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/Frame.cpp:61: ASSERT(!m_owner); On 2017/03/22 at 23:44:10, dcheng wrote: > Shall we assert that the the lifecycle is in the detached state here? Yup, done. https://codereview.chromium.org/2748603002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/Frame.cpp:75: m_lifecycle.advanceTo(FrameLifecycle::Detaching); On 2017/03/22 at 23:44:10, dcheng wrote: > Instead of advancing to detaching here, can we assert that we are already in the detaching state? Both of the subclasses should have advanced it already. Done & added comment. https://codereview.chromium.org/2748603002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameLifecycle.cpp (right): https://codereview.chromium.org/2748603002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameLifecycle.cpp:14: switch (state) { On 2017/03/22 at 23:44:10, dcheng wrote: > On 2017/03/22 03:05:28, sashab wrote: > > dcheng@ -- think this is the best place for this logic? Wasn't sure whether to > > put this here or in the callers, eg add an if-check to only advance state in > > LocalFrame::detach() if we're not already in the Detaching state. > > This makes sense to me. Ok; left it this way. https://codereview.chromium.org/2748603002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/Page.cpp (right): https://codereview.chromium.org/2748603002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/Page.cpp:645: if (mainFrame->isAttached()) Had to add this, otherwise a few unit tests crash (where the frame is detached and then goes out of scope, and is detached again).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2748603002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/Page.cpp (right): https://codereview.chromium.org/2748603002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/Page.cpp:645: if (mainFrame->isAttached()) On 2017/03/24 02:28:30, sashab wrote: > Had to add this, otherwise a few unit tests crash (where the frame is detached > and then goes out of scope, and is detached again). How bad are the unit tests? I'm OK with this as a temporary thing if we can fix them in a followup.
Added TODO, will fix in follow-up patch.
The CQ bit was checked by sashab@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...
LGTM
The CQ bit was unchecked by sashab@chromium.org
The CQ bit was checked by sashab@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sashab@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2748603002/#ps160001 (title: "Rebase")
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": 160001, "attempt_start_ts": 1490591736516880,
"parent_rev": "24acb2c9fcaeef8e5b30b82e5b2fdaa9396353c7", "commit_rev":
"382a2483590ea3a82082d78c36b1f10a65ac7501"}
Message was sent while issue was closed.
Description was changed from ========== Add a lifecycle state machine for Frame This CL adds a linear state machine to the Frame to ensure that it follows an orderly progression through its state machine. The plan is to use this state machine to make an isAttached() method which can check this state, and later make page(), client(), host() etc return references that DCHECK on the correct state. Also removed m_isDetaching and replaced it with checks on the lifecycle. Based on abarth's CL for adding a state machine for Document in: https://chromiumcodereview.appspot.com/25036004 BUG=700783 ========== to ========== Add a lifecycle state machine for Frame This CL adds a linear state machine to the Frame to ensure that it follows an orderly progression through its state machine. The plan is to use this state machine to make an isAttached() method which can check this state, and later make page(), client(), host() etc return references that DCHECK on the correct state. Also removed m_isDetaching and replaced it with checks on the lifecycle. Based on abarth's CL for adding a state machine for Document in: https://chromiumcodereview.appspot.com/25036004 BUG=700783 Review-Url: https://codereview.chromium.org/2748603002 Cr-Commit-Position: refs/heads/master@{#459710} Committed: https://chromium.googlesource.com/chromium/src/+/382a2483590ea3a82082d78c36b1... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/382a2483590ea3a82082d78c36b1... |
