Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1290053003:
Handle LocalFrame being nestedly detached by subframe. (Closed)
DescriptionHandle LocalFrame being nestedly detached by subframe.
As part of detaching a LocalFrame, its subframes are also detached. Should the
detachment of those subframes trigger a nested detach of the LocalFrame that
initiated their detachment, the LocalFrame being returned to after having
detached the subframes could well be in a detached state. The detach steps that
followed (FrameLoader operations) weren't prepared for being in a detached state
(following r199143) and failed.
And the FrameLoader shouldn't have to gracefully handle being used in a detached
state, so add an is-detached check to the LocalFrame's detach steps before continuing.
Leaving early if so.
R=dcheng,japhet
BUG=520014
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=200820
Patch Set 1 #
Total comments: 2
Patch Set 2 : improve test naming #Patch Set 3 : experiment: add another nested detach check #
Total comments: 2
Patch Set 4 : reposition detach check in LocalFrame::detach() #Patch Set 5 : go back to ps#3 #Patch Set 6 : rebased #
Messages
Total messages: 20 (3 generated)
sigbjornf@opera.com changed reviewers: + dcheng@chromium.org, japhet@chromium.org
please take a look. It's either this or add an earlier "!client()" check in LocalFrame::detach() to detect already detached frames. I went with the former.
lgtm
LGTM with a question... https://codereview.chromium.org/1290053003/diff/1/Source/core/loader/FrameLoa... File Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/1290053003/diff/1/Source/core/loader/FrameLoa... Source/core/loader/FrameLoader.cpp:482: if (m_progressTracker) Is there any work that needs to be done in the frame-already-detached case? Can we early-exit at the top of the function instead?
On 2015/08/17 17:01:58, Nate Chapin wrote: > LGTM with a question... > > https://codereview.chromium.org/1290053003/diff/1/Source/core/loader/FrameLoa... > File Source/core/loader/FrameLoader.cpp (right): > > https://codereview.chromium.org/1290053003/diff/1/Source/core/loader/FrameLoa... > Source/core/loader/FrameLoader.cpp:482: if (m_progressTracker) > Is there any work that needs to be done in the frame-already-detached case? Can > we early-exit at the top of the function instead? A quick experiment for the early-exit strategy, sans test: https://codereview.chromium.org/1300693002/ See also https://codereview.chromium.org/1298643003/, which discusses a different crash with a similar premise.
https://codereview.chromium.org/1290053003/diff/1/Source/core/loader/FrameLoa... File Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/1290053003/diff/1/Source/core/loader/FrameLoa... Source/core/loader/FrameLoader.cpp:482: if (m_progressTracker) On 2015/08/17 17:01:58, Nate Chapin wrote: > Is there any work that needs to be done in the frame-already-detached case? Can > we early-exit at the top of the function instead? It won't do any work before the detach check just below, so you could insert another one at the top here. This is related to #2 -- it seems preferable to head off repeated LocalFrame detaches at the frame level, rather than at the auxiliary FrameLoader.
On 2015/08/18 05:24:13, sof wrote: > https://codereview.chromium.org/1290053003/diff/1/Source/core/loader/FrameLoa... > File Source/core/loader/FrameLoader.cpp (right): > > https://codereview.chromium.org/1290053003/diff/1/Source/core/loader/FrameLoa... > Source/core/loader/FrameLoader.cpp:482: if (m_progressTracker) > On 2015/08/17 17:01:58, Nate Chapin wrote: > > Is there any work that needs to be done in the frame-already-detached case? > Can > > we early-exit at the top of the function instead? > > It won't do any work before the detach check just below, so you could insert > another one at the top here. > > This is related to #2 -- it seems preferable to head off repeated LocalFrame > detaches at the frame level, rather than at the auxiliary FrameLoader. iow, align with https://codereview.chromium.org/645623004/ and try to keep detaches in one spot. I think that's a sane story we should try to hold onto :) The presence of two calls to stopAllParsing() in LocalFrame::detach() confused me wrt cause here -- it is a nested detach going wrong, looking at it more carefully. (The detachChildren() call will detach the parent frame, causing the latter stopAllParsing() call to fail.) I've renamed the test to reflect this.
On 2015/08/18 15:23:53, sof wrote: > On 2015/08/18 05:24:13, sof wrote: > > > https://codereview.chromium.org/1290053003/diff/1/Source/core/loader/FrameLoa... > > File Source/core/loader/FrameLoader.cpp (right): > > > > > https://codereview.chromium.org/1290053003/diff/1/Source/core/loader/FrameLoa... > > Source/core/loader/FrameLoader.cpp:482: if (m_progressTracker) > > On 2015/08/17 17:01:58, Nate Chapin wrote: > > > Is there any work that needs to be done in the frame-already-detached case? > > Can > > > we early-exit at the top of the function instead? > > > > It won't do any work before the detach check just below, so you could insert > > another one at the top here. > > > > This is related to #2 -- it seems preferable to head off repeated LocalFrame > > detaches at the frame level, rather than at the auxiliary FrameLoader. > > iow, align with https://codereview.chromium.org/645623004/ and try to keep > detaches in one spot. I think that's a sane story we should try to hold onto :) > > The presence of two calls to stopAllParsing() in LocalFrame::detach() confused > me wrt cause here -- it is a nested detach going wrong, looking at it more > carefully. (The detachChildren() call will detach the parent frame, causing the > latter stopAllParsing() call to fail.) > > I've renamed the test to reflect this. I guess that's ok. It just feels a little silly to me to be null checking ~everything in finishedParsing() in the meantime.
On 2015/08/18 17:53:16, Nate Chapin wrote: > On 2015/08/18 15:23:53, sof wrote: > > On 2015/08/18 05:24:13, sof wrote: > > > > > > https://codereview.chromium.org/1290053003/diff/1/Source/core/loader/FrameLoa... > > > File Source/core/loader/FrameLoader.cpp (right): > > > > > > > > > https://codereview.chromium.org/1290053003/diff/1/Source/core/loader/FrameLoa... > > > Source/core/loader/FrameLoader.cpp:482: if (m_progressTracker) > > > On 2015/08/17 17:01:58, Nate Chapin wrote: > > > > Is there any work that needs to be done in the frame-already-detached > case? > > > Can > > > > we early-exit at the top of the function instead? > > > > > > It won't do any work before the detach check just below, so you could insert > > > another one at the top here. > > > > > > This is related to #2 -- it seems preferable to head off repeated LocalFrame > > > detaches at the frame level, rather than at the auxiliary FrameLoader. > > > > iow, align with https://codereview.chromium.org/645623004/ and try to keep > > detaches in one spot. I think that's a sane story we should try to hold onto > :) > > > > The presence of two calls to stopAllParsing() in LocalFrame::detach() confused > > me wrt cause here -- it is a nested detach going wrong, looking at it more > > carefully. (The detachChildren() call will detach the parent frame, causing > the > > latter stopAllParsing() call to fail.) > > > > I've renamed the test to reflect this. > > I guess that's ok. It just feels a little silly to me to be null checking > ~everything in finishedParsing() in the meantime. If the owning LocalFrame is responsible for only calling these FrameLoader methods on a non-detached FrameLoader, those can clearly be removed. Is it your preference that we do that or do you want to add initial is-detached calls on FrameLoader methods (again)? It's unclear if you want to continue what https://codereview.chromium.org/645623004/ pushed for or not.
On 2015/08/18 18:01:39, sof wrote: > On 2015/08/18 17:53:16, Nate Chapin wrote: > > On 2015/08/18 15:23:53, sof wrote: > > > On 2015/08/18 05:24:13, sof wrote: > > > > > > > > > > https://codereview.chromium.org/1290053003/diff/1/Source/core/loader/FrameLoa... > > > > File Source/core/loader/FrameLoader.cpp (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1290053003/diff/1/Source/core/loader/FrameLoa... > > > > Source/core/loader/FrameLoader.cpp:482: if (m_progressTracker) > > > > On 2015/08/17 17:01:58, Nate Chapin wrote: > > > > > Is there any work that needs to be done in the frame-already-detached > > case? > > > > Can > > > > > we early-exit at the top of the function instead? > > > > > > > > It won't do any work before the detach check just below, so you could > insert > > > > another one at the top here. > > > > > > > > This is related to #2 -- it seems preferable to head off repeated > LocalFrame > > > > detaches at the frame level, rather than at the auxiliary FrameLoader. > > > > > > iow, align with https://codereview.chromium.org/645623004/ and try to keep > > > detaches in one spot. I think that's a sane story we should try to hold onto > > :) > > > > > > The presence of two calls to stopAllParsing() in LocalFrame::detach() > confused > > > me wrt cause here -- it is a nested detach going wrong, looking at it more > > > carefully. (The detachChildren() call will detach the parent frame, causing > > the > > > latter stopAllParsing() call to fail.) > > > > > > I've renamed the test to reflect this. > > > > I guess that's ok. It just feels a little silly to me to be null checking > > ~everything in finishedParsing() in the meantime. > > If the owning LocalFrame is responsible for only calling these FrameLoader > methods on a non-detached FrameLoader, those can clearly be removed. Is it your > preference that we do that or do you want to add initial is-detached calls on > FrameLoader methods (again)? It's unclear if you want to continue what > https://codereview.chromium.org/645623004/ pushed for or not. If LocalFrame::detach can skip calling FrameLoader methods when FrameLoader is already detached and that fixes all the crashes, that's probably the best case scenario? However, I was thinking that it was possible to get this kind of crash due to bad behavior inside FrameLoader::stopAllLoaders, without LocalFrame::detach() being on the stack. I guess I don't have any evidence of that, though. Just a vague paranoia :) All that said, I'm not opposed to landing this as-is and thinking more about this in the future.
On 2015/08/18 18:15:39, Nate Chapin wrote: > On 2015/08/18 18:01:39, sof wrote: > > On 2015/08/18 17:53:16, Nate Chapin wrote: > > > On 2015/08/18 15:23:53, sof wrote: > > > > On 2015/08/18 05:24:13, sof wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/1290053003/diff/1/Source/core/loader/FrameLoa... > > > > > File Source/core/loader/FrameLoader.cpp (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1290053003/diff/1/Source/core/loader/FrameLoa... > > > > > Source/core/loader/FrameLoader.cpp:482: if (m_progressTracker) > > > > > On 2015/08/17 17:01:58, Nate Chapin wrote: > > > > > > Is there any work that needs to be done in the frame-already-detached > > > case? > > > > > Can > > > > > > we early-exit at the top of the function instead? > > > > > > > > > > It won't do any work before the detach check just below, so you could > > insert > > > > > another one at the top here. > > > > > > > > > > This is related to #2 -- it seems preferable to head off repeated > > LocalFrame > > > > > detaches at the frame level, rather than at the auxiliary FrameLoader. > > > > > > > > iow, align with https://codereview.chromium.org/645623004/ and try to keep > > > > detaches in one spot. I think that's a sane story we should try to hold > onto > > > :) > > > > > > > > The presence of two calls to stopAllParsing() in LocalFrame::detach() > > confused > > > > me wrt cause here -- it is a nested detach going wrong, looking at it more > > > > carefully. (The detachChildren() call will detach the parent frame, > causing > > > the > > > > latter stopAllParsing() call to fail.) > > > > > > > > I've renamed the test to reflect this. > > > > > > I guess that's ok. It just feels a little silly to me to be null checking > > > ~everything in finishedParsing() in the meantime. > > > > If the owning LocalFrame is responsible for only calling these FrameLoader > > methods on a non-detached FrameLoader, those can clearly be removed. Is it > your > > preference that we do that or do you want to add initial is-detached calls on > > FrameLoader methods (again)? It's unclear if you want to continue what > > https://codereview.chromium.org/645623004/ pushed for or not. > > If LocalFrame::detach can skip calling FrameLoader methods when FrameLoader is > already detached and that fixes all the crashes, that's probably the best case > scenario? > Agreed, let me try that overnight. If that will take longer to settle down, I think we ought to land this CL as-is as there's a branchpoint this week (or next) ?
On 2015/08/18 19:24:51, sof wrote: > On 2015/08/18 18:15:39, Nate Chapin wrote: > > On 2015/08/18 18:01:39, sof wrote: > > > On 2015/08/18 17:53:16, Nate Chapin wrote: > > > > On 2015/08/18 15:23:53, sof wrote: > > > > > On 2015/08/18 05:24:13, sof wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1290053003/diff/1/Source/core/loader/FrameLoa... > > > > > > File Source/core/loader/FrameLoader.cpp (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1290053003/diff/1/Source/core/loader/FrameLoa... > > > > > > Source/core/loader/FrameLoader.cpp:482: if (m_progressTracker) > > > > > > On 2015/08/17 17:01:58, Nate Chapin wrote: > > > > > > > Is there any work that needs to be done in the > frame-already-detached > > > > case? > > > > > > Can > > > > > > > we early-exit at the top of the function instead? > > > > > > > > > > > > It won't do any work before the detach check just below, so you could > > > insert > > > > > > another one at the top here. > > > > > > > > > > > > This is related to #2 -- it seems preferable to head off repeated > > > LocalFrame > > > > > > detaches at the frame level, rather than at the auxiliary FrameLoader. > > > > > > > > > > iow, align with https://codereview.chromium.org/645623004/ and try to > keep > > > > > detaches in one spot. I think that's a sane story we should try to hold > > onto > > > > :) > > > > > > > > > > The presence of two calls to stopAllParsing() in LocalFrame::detach() > > > confused > > > > > me wrt cause here -- it is a nested detach going wrong, looking at it > more > > > > > carefully. (The detachChildren() call will detach the parent frame, > > causing > > > > the > > > > > latter stopAllParsing() call to fail.) > > > > > > > > > > I've renamed the test to reflect this. > > > > > > > > I guess that's ok. It just feels a little silly to me to be null checking > > > > ~everything in finishedParsing() in the meantime. > > > > > > If the owning LocalFrame is responsible for only calling these FrameLoader > > > methods on a non-detached FrameLoader, those can clearly be removed. Is it > > your > > > preference that we do that or do you want to add initial is-detached calls > on > > > FrameLoader methods (again)? It's unclear if you want to continue what > > > https://codereview.chromium.org/645623004/ pushed for or not. > > > > If LocalFrame::detach can skip calling FrameLoader methods when FrameLoader is > > already detached and that fixes all the crashes, that's probably the best case > > scenario? > > > > Agreed, let me try that overnight. If that will take longer to settle down, I > think we ought to land this CL as-is as there's a branchpoint this week (or > next) ? Sounds good. LGTM to land.
On 2015/08/18 20:59:55, Nate Chapin wrote: > On 2015/08/18 19:24:51, sof wrote: > > On 2015/08/18 18:15:39, Nate Chapin wrote: > > > On 2015/08/18 18:01:39, sof wrote: > > > > On 2015/08/18 17:53:16, Nate Chapin wrote: > > > > > On 2015/08/18 15:23:53, sof wrote: > > > > > > On 2015/08/18 05:24:13, sof wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1290053003/diff/1/Source/core/loader/FrameLoa... > > > > > > > File Source/core/loader/FrameLoader.cpp (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1290053003/diff/1/Source/core/loader/FrameLoa... > > > > > > > Source/core/loader/FrameLoader.cpp:482: if (m_progressTracker) > > > > > > > On 2015/08/17 17:01:58, Nate Chapin wrote: > > > > > > > > Is there any work that needs to be done in the > > frame-already-detached > > > > > case? > > > > > > > Can > > > > > > > > we early-exit at the top of the function instead? > > > > > > > > > > > > > > It won't do any work before the detach check just below, so you > could > > > > insert > > > > > > > another one at the top here. > > > > > > > > > > > > > > This is related to #2 -- it seems preferable to head off repeated > > > > LocalFrame > > > > > > > detaches at the frame level, rather than at the auxiliary > FrameLoader. > > > > > > > > > > > > iow, align with https://codereview.chromium.org/645623004/ and try to > > keep > > > > > > detaches in one spot. I think that's a sane story we should try to > hold > > > onto > > > > > :) > > > > > > > > > > > > The presence of two calls to stopAllParsing() in LocalFrame::detach() > > > > confused > > > > > > me wrt cause here -- it is a nested detach going wrong, looking at it > > more > > > > > > carefully. (The detachChildren() call will detach the parent frame, > > > causing > > > > > the > > > > > > latter stopAllParsing() call to fail.) > > > > > > > > > > > > I've renamed the test to reflect this. > > > > > > > > > > I guess that's ok. It just feels a little silly to me to be null > checking > > > > > ~everything in finishedParsing() in the meantime. > > > > > > > > If the owning LocalFrame is responsible for only calling these FrameLoader > > > > methods on a non-detached FrameLoader, those can clearly be removed. Is it > > > your > > > > preference that we do that or do you want to add initial is-detached calls > > on > > > > FrameLoader methods (again)? It's unclear if you want to continue what > > > > https://codereview.chromium.org/645623004/ pushed for or not. > > > > > > If LocalFrame::detach can skip calling FrameLoader methods when FrameLoader > is > > > already detached and that fixes all the crashes, that's probably the best > case > > > scenario? > > > > > > > Agreed, let me try that overnight. If that will take longer to settle down, I > > think we ought to land this CL as-is as there's a branchpoint this week (or > > next) ? > > Sounds good. LGTM to land. ps#3 adds that detach check instead; looks ready.
On 2015/08/18 21:17:41, sof wrote: > On 2015/08/18 20:59:55, Nate Chapin wrote: > > On 2015/08/18 19:24:51, sof wrote: > > > On 2015/08/18 18:15:39, Nate Chapin wrote: > > > > On 2015/08/18 18:01:39, sof wrote: > > > > > On 2015/08/18 17:53:16, Nate Chapin wrote: > > > > > > On 2015/08/18 15:23:53, sof wrote: > > > > > > > On 2015/08/18 05:24:13, sof wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1290053003/diff/1/Source/core/loader/FrameLoa... > > > > > > > > File Source/core/loader/FrameLoader.cpp (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1290053003/diff/1/Source/core/loader/FrameLoa... > > > > > > > > Source/core/loader/FrameLoader.cpp:482: if (m_progressTracker) > > > > > > > > On 2015/08/17 17:01:58, Nate Chapin wrote: > > > > > > > > > Is there any work that needs to be done in the > > > frame-already-detached > > > > > > case? > > > > > > > > Can > > > > > > > > > we early-exit at the top of the function instead? > > > > > > > > > > > > > > > > It won't do any work before the detach check just below, so you > > could > > > > > insert > > > > > > > > another one at the top here. > > > > > > > > > > > > > > > > This is related to #2 -- it seems preferable to head off repeated > > > > > LocalFrame > > > > > > > > detaches at the frame level, rather than at the auxiliary > > FrameLoader. > > > > > > > > > > > > > > iow, align with https://codereview.chromium.org/645623004/ and try > to > > > keep > > > > > > > detaches in one spot. I think that's a sane story we should try to > > hold > > > > onto > > > > > > :) > > > > > > > > > > > > > > The presence of two calls to stopAllParsing() in > LocalFrame::detach() > > > > > confused > > > > > > > me wrt cause here -- it is a nested detach going wrong, looking at > it > > > more > > > > > > > carefully. (The detachChildren() call will detach the parent frame, > > > > causing > > > > > > the > > > > > > > latter stopAllParsing() call to fail.) > > > > > > > > > > > > > > I've renamed the test to reflect this. > > > > > > > > > > > > I guess that's ok. It just feels a little silly to me to be null > > checking > > > > > > ~everything in finishedParsing() in the meantime. > > > > > > > > > > If the owning LocalFrame is responsible for only calling these > FrameLoader > > > > > methods on a non-detached FrameLoader, those can clearly be removed. Is > it > > > > your > > > > > preference that we do that or do you want to add initial is-detached > calls > > > on > > > > > FrameLoader methods (again)? It's unclear if you want to continue what > > > > > https://codereview.chromium.org/645623004/ pushed for or not. > > > > > > > > If LocalFrame::detach can skip calling FrameLoader methods when > FrameLoader > > is > > > > already detached and that fixes all the crashes, that's probably the best > > case > > > > scenario? > > > > > > > > > > Agreed, let me try that overnight. If that will take longer to settle down, > I > > > think we ought to land this CL as-is as there's a branchpoint this week (or > > > next) ? > > > > Sounds good. LGTM to land. > > ps#3 adds that detach check instead; looks ready. Awesome. LGTM
https://codereview.chromium.org/1290053003/diff/40001/Source/core/frame/Local... File Source/core/frame/LocalFrame.cpp (right): https://codereview.chromium.org/1290053003/diff/40001/Source/core/frame/Local... Source/core/frame/LocalFrame.cpp:296: if (!client()) Out of curiosity, do we still need this check with the above check? The only thing that should be able to detach frames at this point is WebPlugin nodes being detached in Document::detach()... but Document::detach() already blocks scripting. So in theory... it might work without this? (I'd be OK with deferring this to a followup patch, but I think it's worthy of adding a comment to investigate this more)
Thanks for the good questions, landing. https://codereview.chromium.org/1290053003/diff/40001/Source/core/frame/Local... File Source/core/frame/LocalFrame.cpp (right): https://codereview.chromium.org/1290053003/diff/40001/Source/core/frame/Local... Source/core/frame/LocalFrame.cpp:296: if (!client()) On 2015/08/18 21:26:09, dcheng wrote: > Out of curiosity, do we still need this check with the above check? The only > thing that should be able to detach frames at this point is WebPlugin nodes > being detached in Document::detach()... but Document::detach() already blocks > scripting. So in theory... it might work without this? > > (I'd be OK with deferring this to a followup patch, but I think it's worthy of > adding a comment to investigate this more) We do still need that one, added in that spot by issue 452253, as it catches detaches that happen due to resource load cancellations. (See ps#4 bot results for what happens if you do remove it.)
The CQ bit was checked by sigbjornf@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org, japhet@chromium.org Link to the patchset: https://codereview.chromium.org/1290053003/#ps100001 (title: "rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1290053003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1290053003/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://src.chromium.org/viewvc/blink?view=rev&revision=200820 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
