Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1298643003:
Fixes crash when swapping frames (Closed)
DescriptionFixes crash when swapping frames
m_documentLoader is null. Here's the trace:
html_viewer.mojo!blink::DocumentLoader::isCommittedButEmpty() Line 105
html_viewer.mojo!blink::FrameLoader::finishedParsing() Line 485
html_viewer.mojo!blink::FrameLoader::stopAllLoaders() Line 981
html_viewer.mojo!blink::LocalFrame::detach(blink::FrameDetachType type) Line 281
html_viewer.mojo!blink::WebFrame::swap(blink::WebFrame * frame) Line 114
BUG=521663
TEST=none
R=japhet@chromium.org
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201027
Patch Set 1 #
Total comments: 2
Messages
Total messages: 26 (2 generated)
I'm not familiar with this code. It's entirely possible I'm papering over some other bad condition. If you think that is the case, any suggestions for what might be going wrong?
FWIW, a crash that is similar in concept is being reviewed in https://codereview.chromium.org/1290053003/. If my early-exit theory in that bug is correct, than this crash should get fixed by it as well. https://codereview.chromium.org/1298643003/diff/1/Source/core/loader/FrameLoa... File Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/1298643003/diff/1/Source/core/loader/FrameLoa... Source/core/loader/FrameLoader.cpp:1258: || (documentLoader() && !documentLoader()->initialScrollState().didRestoreFromHistory); Is this part actually necessary? I would have thought the FrameView null check at the top of this function would catch the null DocumentLoader case.
https://codereview.chromium.org/1298643003/diff/1/Source/core/loader/FrameLoa... File Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/1298643003/diff/1/Source/core/loader/FrameLoa... Source/core/loader/FrameLoader.cpp:1258: || (documentLoader() && !documentLoader()->initialScrollState().didRestoreFromHistory); On 2015/08/17 19:20:35, Nate Chapin wrote: > Is this part actually necessary? I would have thought the FrameView null check > at the top of this function would catch the null DocumentLoader case. My first fix was line 485. That fix got me here. So, yes, both were necessary. The view is non-null. If it were null we wouldn't have gotten here because of the conditional on 489.
On 2015/08/17 19:20:35, Nate Chapin wrote: > FWIW, a crash that is similar in concept is being reviewed in > https://codereview.chromium.org/1290053003/. If my early-exit theory in that bug > is correct, than this crash should get fixed by it as well. What is the condition for the early out?
On 2015/08/17 20:34:57, sky wrote: > On 2015/08/17 19:20:35, Nate Chapin wrote: > > FWIW, a crash that is similar in concept is being reviewed in > > https://codereview.chromium.org/1290053003/. If my early-exit theory in that > bug > > is correct, than this crash should get fixed by it as well. > > What is the condition for the early out? Probably null-checking m_documentLoader. It's one of the first things that gets cleared during the detach process. I don't know for certain that it won't cause regressions, though. I may experiment with that this afternoon if I have time.
On 2015/08/17 20:39:11, Nate Chapin wrote: > On 2015/08/17 20:34:57, sky wrote: > > On 2015/08/17 19:20:35, Nate Chapin wrote: > > > FWIW, a crash that is similar in concept is being reviewed in > > > https://codereview.chromium.org/1290053003/. If my early-exit theory in that > > bug > > > is correct, than this crash should get fixed by it as well. > > > > What is the condition for the early out? > > Probably null-checking m_documentLoader. It's one of the first things that gets > cleared during the detach process. I don't know for certain that it won't cause > regressions, though. I may experiment with that this afternoon if I have time. The null check for m_documentLoader works for me. I have no idea of the side effects though. How would you recommend I proceed?
On 2015/08/17 21:10:18, sky wrote: > On 2015/08/17 20:39:11, Nate Chapin wrote: > > On 2015/08/17 20:34:57, sky wrote: > > > On 2015/08/17 19:20:35, Nate Chapin wrote: > > > > FWIW, a crash that is similar in concept is being reviewed in > > > > https://codereview.chromium.org/1290053003/. If my early-exit theory in > that > > > bug > > > > is correct, than this crash should get fixed by it as well. > > > > > > What is the condition for the early out? > > > > Probably null-checking m_documentLoader. It's one of the first things that > gets > > cleared during the detach process. I don't know for certain that it won't > cause > > regressions, though. I may experiment with that this afternoon if I have time. > > The null check for m_documentLoader works for me. I have no idea of the side > effects though. How would you recommend I proceed? Let's see what the trybots say: https://codereview.chromium.org/1300693002/
dcheng@chromium.org changed reviewers: + dcheng@chromium.org
I'd like to understand why there's a difference between content/ and mojo/ here. Is it possible that Mojo is explicitly detaching the frame before trying to swap() it? swap() is a pretty tricky interface, so I've set a reminder to add some documentation tomorrow to WebFrame::swap(). At the same time, I'm not sure how manageable just documenting everything in method comments is going to be: maybe we need a 'life of a frame' overview in WebFrame.h?
On 2015/08/18 05:30:16, dcheng wrote: > I'd like to understand why there's a difference between content/ and mojo/ here. > > Is it possible that Mojo is explicitly detaching the frame before trying to > swap() it? > > swap() is a pretty tricky interface, so I've set a reminder to add some > documentation tomorrow to WebFrame::swap(). At the same time, I'm not sure how > manageable just documenting everything in method comments is going to be: maybe > we need a 'life of a frame' overview in WebFrame.h? I think this is a general detach bug, not specific to mojo or WebFrame::swap. See https://code.google.com/p/chromium/issues/detail?id=510548
On Mon, Aug 17, 2015 at 2:31 PM, <japhet@chromium.org> wrote: > On 2015/08/17 21:10:18, sky wrote: >> >> On 2015/08/17 20:39:11, Nate Chapin wrote: >> > On 2015/08/17 20:34:57, sky wrote: >> > > On 2015/08/17 19:20:35, Nate Chapin wrote: >> > > > FWIW, a crash that is similar in concept is being reviewed in >> > > > https://codereview.chromium.org/1290053003/. If my early-exit theory >> > > > in >> that >> > > bug >> > > > is correct, than this crash should get fixed by it as well. >> > > >> > > What is the condition for the early out? >> > >> > Probably null-checking m_documentLoader. It's one of the first things >> > that >> gets >> > cleared during the detach process. I don't know for certain that it >> > won't >> cause >> > regressions, though. I may experiment with that this afternoon if I have > > time. > >> The null check for m_documentLoader works for me. I have no idea of the >> side >> effects though. How would you recommend I proceed? > > > Let's see what the trybots say: https://codereview.chromium.org/1300693002/ Am I reading the trybot output right in that they didn't like the patch? Is there something I can do to help with this? Thanks, -Scott > > https://codereview.chromium.org/1298643003/ To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
On 2015/08/18 18:15:43, sky wrote: > On Mon, Aug 17, 2015 at 2:31 PM, <mailto:japhet@chromium.org> wrote: > > On 2015/08/17 21:10:18, sky wrote: > >> > >> On 2015/08/17 20:39:11, Nate Chapin wrote: > >> > On 2015/08/17 20:34:57, sky wrote: > >> > > On 2015/08/17 19:20:35, Nate Chapin wrote: > >> > > > FWIW, a crash that is similar in concept is being reviewed in > >> > > > https://codereview.chromium.org/1290053003/. If my early-exit theory > >> > > > in > >> that > >> > > bug > >> > > > is correct, than this crash should get fixed by it as well. > >> > > > >> > > What is the condition for the early out? > >> > > >> > Probably null-checking m_documentLoader. It's one of the first things > >> > that > >> gets > >> > cleared during the detach process. I don't know for certain that it > >> > won't > >> cause > >> > regressions, though. I may experiment with that this afternoon if I have > > > > time. > > > >> The null check for m_documentLoader works for me. I have no idea of the > >> side > >> effects though. How would you recommend I proceed? > > > > > > Let's see what the trybots say: https://codereview.chromium.org/1300693002/ > > Am I reading the trybot output right in that they didn't like the > patch? Is there something I can do to help with this? > > Thanks, > > -Scott It looks like several bots freaked out, so I restarted those jobs. > > > > > https://codereview.chromium.org/1298643003/ > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:blink-reviews+unsubscribe@chromium.org.
FYI, https://codereview.chromium.org/1290053003 was updated and probably(?) fixes this crash as well.
On 2015/08/19 22:22:29, Nate Chapin wrote: > FYI, https://codereview.chromium.org/1290053003 was updated and probably(?) > fixes this crash as well. I still get the crash. Here's the stack: html_viewer.mojo!blink::DocumentLoader::isCommittedButEmpty() Line 107 C++ html_viewer.mojo!blink::FrameLoader::finishedParsing() Line 485 C++ html_viewer.mojo!blink::FrameLoader::stopAllLoaders() Line 980 C++ > html_viewer.mojo!blink::LocalFrame::detach(blink::FrameDetachType type) Line 282 C++ The early out in detach(FrameDetachType type) isn't early enough. Should I proceed with this one?
On 2015/08/19 23:18:05, sky wrote: > On 2015/08/19 22:22:29, Nate Chapin wrote: > > FYI, https://codereview.chromium.org/1290053003 was updated and probably(?) > > fixes this crash as well. > > I still get the crash. Here's the stack: > > html_viewer.mojo!blink::DocumentLoader::isCommittedButEmpty() Line 107 C++ > html_viewer.mojo!blink::FrameLoader::finishedParsing() Line 485 C++ > html_viewer.mojo!blink::FrameLoader::stopAllLoaders() Line 980 C++ > > html_viewer.mojo!blink::LocalFrame::detach(blink::FrameDetachType type) Line > 282 C++ > > > The early out in detach(FrameDetachType type) isn't early enough. Should I > proceed with this one? Do you have repro steps or a test case handy? I'd be interested to see exactly how we're getting into this state.
On 2015/08/20 00:02:57, Nate Chapin wrote: > On 2015/08/19 23:18:05, sky wrote: > > On 2015/08/19 22:22:29, Nate Chapin wrote: > > > FYI, https://codereview.chromium.org/1290053003 was updated and probably(?) > > > fixes this crash as well. > > > > I still get the crash. Here's the stack: > > > > html_viewer.mojo!blink::DocumentLoader::isCommittedButEmpty() Line 107 C++ > > html_viewer.mojo!blink::FrameLoader::finishedParsing() Line 485 C++ > > html_viewer.mojo!blink::FrameLoader::stopAllLoaders() Line 980 C++ > > > html_viewer.mojo!blink::LocalFrame::detach(blink::FrameDetachType type) Line > > 282 C++ > > > > > > The early out in detach(FrameDetachType type) isn't early enough. Should I > > proceed with this one? > > Do you have repro steps or a test case handy? I'd be interested to see exactly > how we're getting into this state. Only with mandoline. Build instructions are here: https://www.chromium.org/developers/mandoline/build (basically gn only, and *not* component build). Once you build mandoline try displaying a page like http://amazon.com/ and chances are you'll hit the crash. It's entirely possible I'm doing something wrong too, not sure.
On 2015/08/20 00:04:36, sky wrote: > On 2015/08/20 00:02:57, Nate Chapin wrote: > > On 2015/08/19 23:18:05, sky wrote: > > > On 2015/08/19 22:22:29, Nate Chapin wrote: > > > > FYI, https://codereview.chromium.org/1290053003 was updated and > probably(?) > > > > fixes this crash as well. > > > > > > I still get the crash. Here's the stack: > > > > > > html_viewer.mojo!blink::DocumentLoader::isCommittedButEmpty() Line 107 C++ > > > html_viewer.mojo!blink::FrameLoader::finishedParsing() Line 485 C++ > > > html_viewer.mojo!blink::FrameLoader::stopAllLoaders() Line 980 C++ > > > > html_viewer.mojo!blink::LocalFrame::detach(blink::FrameDetachType type) > Line > > > 282 C++ > > > > > > > > > The early out in detach(FrameDetachType type) isn't early enough. Should I > > > proceed with this one? > > > > Do you have repro steps or a test case handy? I'd be interested to see exactly > > how we're getting into this state. > > Only with mandoline. Build instructions are here: > https://www.chromium.org/developers/mandoline/build (basically gn only, and > *not* component build). Once you build mandoline try displaying a page like > http://amazon.com/ and chances are you'll hit the crash. > It's entirely possible I'm doing something wrong too, not sure. Nate, any update on this? Others are hitting the bug and a bit blocked by it. Thanks, -Scott
On 2015/08/21 19:04:45, sky wrote: > On 2015/08/20 00:04:36, sky wrote: > > On 2015/08/20 00:02:57, Nate Chapin wrote: > > > On 2015/08/19 23:18:05, sky wrote: > > > > On 2015/08/19 22:22:29, Nate Chapin wrote: > > > > > FYI, https://codereview.chromium.org/1290053003 was updated and > > probably(?) > > > > > fixes this crash as well. > > > > > > > > I still get the crash. Here's the stack: > > > > > > > > html_viewer.mojo!blink::DocumentLoader::isCommittedButEmpty() Line > 107 C++ > > > > html_viewer.mojo!blink::FrameLoader::finishedParsing() Line 485 C++ > > > > html_viewer.mojo!blink::FrameLoader::stopAllLoaders() Line 980 C++ > > > > > html_viewer.mojo!blink::LocalFrame::detach(blink::FrameDetachType type) > > Line > > > > 282 C++ > > > > > > > > > > > > The early out in detach(FrameDetachType type) isn't early enough. Should I > > > > proceed with this one? > > > > > > Do you have repro steps or a test case handy? I'd be interested to see > exactly > > > how we're getting into this state. > > > > Only with mandoline. Build instructions are here: > > https://www.chromium.org/developers/mandoline/build (basically gn only, and > > *not* component build). Once you build mandoline try displaying a page like > > http://amazon.com/ and chances are you'll hit the crash. > > It's entirely possible I'm doing something wrong too, not sure. > > Nate, any update on this? Others are hitting the bug and a bit blocked by it. > Thanks, > > -Scott Sorry, LGTM. There are cleanup opportunities, but I'll keep thinking about them rather than blocking you on it.
On 2015/08/21 21:50:33, Nate Chapin wrote: > On 2015/08/21 19:04:45, sky wrote: > > On 2015/08/20 00:04:36, sky wrote: > > > On 2015/08/20 00:02:57, Nate Chapin wrote: > > > > On 2015/08/19 23:18:05, sky wrote: > > > > > On 2015/08/19 22:22:29, Nate Chapin wrote: > > > > > > FYI, https://codereview.chromium.org/1290053003 was updated and > > > probably(?) > > > > > > fixes this crash as well. > > > > > > > > > > I still get the crash. Here's the stack: > > > > > > > > > > html_viewer.mojo!blink::DocumentLoader::isCommittedButEmpty() Line > > 107 C++ > > > > > html_viewer.mojo!blink::FrameLoader::finishedParsing() Line 485 C++ > > > > > html_viewer.mojo!blink::FrameLoader::stopAllLoaders() Line 980 C++ > > > > > > html_viewer.mojo!blink::LocalFrame::detach(blink::FrameDetachType > type) > > > Line > > > > > 282 C++ > > > > > > > > > > > > > > > The early out in detach(FrameDetachType type) isn't early enough. Should > I > > > > > proceed with this one? > > > > > > > > Do you have repro steps or a test case handy? I'd be interested to see > > exactly > > > > how we're getting into this state. > > > > > > Only with mandoline. Build instructions are here: > > > https://www.chromium.org/developers/mandoline/build (basically gn only, and > > > *not* component build). Once you build mandoline try displaying a page like > > > http://amazon.com/ and chances are you'll hit the crash. > > > It's entirely possible I'm doing something wrong too, not sure. > > > > Nate, any update on this? Others are hitting the bug and a bit blocked by it. > > Thanks, > > > > -Scott > > Sorry, LGTM. There are cleanup opportunities, but I'll keep thinking about them > rather than blocking you on it. BTW, do we have a test?
On 2015/08/21 21:50:57, Nate Chapin wrote: > On 2015/08/21 21:50:33, Nate Chapin wrote: > > On 2015/08/21 19:04:45, sky wrote: > > > On 2015/08/20 00:04:36, sky wrote: > > > > On 2015/08/20 00:02:57, Nate Chapin wrote: > > > > > On 2015/08/19 23:18:05, sky wrote: > > > > > > On 2015/08/19 22:22:29, Nate Chapin wrote: > > > > > > > FYI, https://codereview.chromium.org/1290053003 was updated and > > > > probably(?) > > > > > > > fixes this crash as well. > > > > > > > > > > > > I still get the crash. Here's the stack: > > > > > > > > > > > > html_viewer.mojo!blink::DocumentLoader::isCommittedButEmpty() Line > > > 107 C++ > > > > > > html_viewer.mojo!blink::FrameLoader::finishedParsing() Line 485 C++ > > > > > > html_viewer.mojo!blink::FrameLoader::stopAllLoaders() Line 980 C++ > > > > > > > html_viewer.mojo!blink::LocalFrame::detach(blink::FrameDetachType > > type) > > > > Line > > > > > > 282 C++ > > > > > > > > > > > > > > > > > > The early out in detach(FrameDetachType type) isn't early enough. > Should > > I > > > > > > proceed with this one? > > > > > > > > > > Do you have repro steps or a test case handy? I'd be interested to see > > > exactly > > > > > how we're getting into this state. > > > > > > > > Only with mandoline. Build instructions are here: > > > > https://www.chromium.org/developers/mandoline/build (basically gn only, > and > > > > *not* component build). Once you build mandoline try displaying a page > like > > > > http://amazon.com/ and chances are you'll hit the crash. > > > > It's entirely possible I'm doing something wrong too, not sure. > > > > > > Nate, any update on this? Others are hitting the bug and a bit blocked by > it. > > > Thanks, > > > > > > -Scott > > > > Sorry, LGTM. There are cleanup opportunities, but I'll keep thinking about > them > > rather than blocking you on it. > > BTW, do we have a test? I don't and am happy to write one if you can point me in the right direction. Are you ok with me landing this and then trying to figure out a test?
On 2015/08/21 22:00:59, sky wrote: > On 2015/08/21 21:50:57, Nate Chapin wrote: > > On 2015/08/21 21:50:33, Nate Chapin wrote: > > > On 2015/08/21 19:04:45, sky wrote: > > > > On 2015/08/20 00:04:36, sky wrote: > > > > > On 2015/08/20 00:02:57, Nate Chapin wrote: > > > > > > On 2015/08/19 23:18:05, sky wrote: > > > > > > > On 2015/08/19 22:22:29, Nate Chapin wrote: > > > > > > > > FYI, https://codereview.chromium.org/1290053003 was updated and > > > > > probably(?) > > > > > > > > fixes this crash as well. > > > > > > > > > > > > > > I still get the crash. Here's the stack: > > > > > > > > > > > > > > html_viewer.mojo!blink::DocumentLoader::isCommittedButEmpty() Line > > > > 107 C++ > > > > > > > html_viewer.mojo!blink::FrameLoader::finishedParsing() Line > 485 C++ > > > > > > > html_viewer.mojo!blink::FrameLoader::stopAllLoaders() Line 980 C++ > > > > > > > > html_viewer.mojo!blink::LocalFrame::detach(blink::FrameDetachType > > > type) > > > > > Line > > > > > > > 282 C++ > > > > > > > > > > > > > > > > > > > > > The early out in detach(FrameDetachType type) isn't early enough. > > Should > > > I > > > > > > > proceed with this one? > > > > > > > > > > > > Do you have repro steps or a test case handy? I'd be interested to see > > > > exactly > > > > > > how we're getting into this state. > > > > > > > > > > Only with mandoline. Build instructions are here: > > > > > https://www.chromium.org/developers/mandoline/build (basically gn only, > > and > > > > > *not* component build). Once you build mandoline try displaying a page > > like > > > > > http://amazon.com/ and chances are you'll hit the crash. > > > > > It's entirely possible I'm doing something wrong too, not sure. > > > > > > > > Nate, any update on this? Others are hitting the bug and a bit blocked by > > it. > > > > Thanks, > > > > > > > > -Scott > > > > > > Sorry, LGTM. There are cleanup opportunities, but I'll keep thinking about > > them > > > rather than blocking you on it. > > > > BTW, do we have a test? > > I don't and am happy to write one if you can point me in the right direction. > Are you ok with me landing this and then trying to figure out a test? Yeah, I think that's ok. Given that this looks like it involves detaching frames inside an event, though, this may be in a gap in our testing coverage. Swapping frames is probably only possible in a unit test at the moment, but depending on JS events to trigger the crash condition strongly implies a layout test.
The CQ bit was checked by sky@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1298643003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1298643003/1
I suspect the unit test would be easiest. Can you point me at some existing oopif unit tests that would help me get started? On Fri, Aug 21, 2015 at 3:07 PM, <japhet@chromium.org> wrote: > On 2015/08/21 22:00:59, sky wrote: >> >> On 2015/08/21 21:50:57, Nate Chapin wrote: >> > On 2015/08/21 21:50:33, Nate Chapin wrote: >> > > On 2015/08/21 19:04:45, sky wrote: >> > > > On 2015/08/20 00:04:36, sky wrote: >> > > > > On 2015/08/20 00:02:57, Nate Chapin wrote: >> > > > > > On 2015/08/19 23:18:05, sky wrote: >> > > > > > > On 2015/08/19 22:22:29, Nate Chapin wrote: >> > > > > > > > FYI, https://codereview.chromium.org/1290053003 was updated >> > > > > > > > and >> > > > > probably(?) >> > > > > > > > fixes this crash as well. >> > > > > > > >> > > > > > > I still get the crash. Here's the stack: >> > > > > > > >> > > > > > > >> > > > > > > html_viewer.mojo!blink::DocumentLoader::isCommittedButEmpty() > > Line >> >> > > > 107 C++ >> > > > > > > html_viewer.mojo!blink::FrameLoader::finishedParsing() >> > > > > > > Line >> 485 C++ >> > > > > > > html_viewer.mojo!blink::FrameLoader::stopAllLoaders() Line > > 980 C++ >> >> > > > > > > >> html_viewer.mojo!blink::LocalFrame::detach(blink::FrameDetachType >> > > type) >> > > > > Line >> > > > > > > 282 C++ >> > > > > > > >> > > > > > > >> > > > > > > The early out in detach(FrameDetachType type) isn't early >> > > > > > > enough. >> > Should >> > > I >> > > > > > > proceed with this one? >> > > > > > >> > > > > > Do you have repro steps or a test case handy? I'd be interested >> > > > > > to > > see >> >> > > > exactly >> > > > > > how we're getting into this state. >> > > > > >> > > > > Only with mandoline. Build instructions are here: >> > > > > https://www.chromium.org/developers/mandoline/build (basically gn > > only, >> >> > and >> > > > > *not* component build). Once you build mandoline try displaying a >> > > > > page >> > like >> > > > > http://amazon.com/ and chances are you'll hit the crash. >> > > > > It's entirely possible I'm doing something wrong too, not sure. >> > > > >> > > > Nate, any update on this? Others are hitting the bug and a bit >> > > > blocked > > by >> >> > it. >> > > > Thanks, >> > > > >> > > > -Scott >> > > >> > > Sorry, LGTM. There are cleanup opportunities, but I'll keep thinking >> > > about >> > them >> > > rather than blocking you on it. >> > >> > BTW, do we have a test? > > >> I don't and am happy to write one if you can point me in the right >> direction. >> Are you ok with me landing this and then trying to figure out a test? > > > Yeah, I think that's ok. Given that this looks like it involves detaching > frames > inside an event, though, this may be in a gap in our testing coverage. > Swapping > frames is probably only possible in a unit test at the moment, but depending > on > JS events to trigger the crash condition strongly implies a layout test. > > https://codereview.chromium.org/1298643003/ To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
There's a bunch in WebFrameTest.cpp: look for WebFrameSwapTest. Daniel On Fri, Aug 21, 2015, 17:14 Scott Violet <sky@chromium.org> wrote: > I suspect the unit test would be easiest. Can you point me at some > existing oopif unit tests that would help me get started? > > On Fri, Aug 21, 2015 at 3:07 PM, <japhet@chromium.org> wrote: > > On 2015/08/21 22:00:59, sky wrote: > >> > >> On 2015/08/21 21:50:57, Nate Chapin wrote: > >> > On 2015/08/21 21:50:33, Nate Chapin wrote: > >> > > On 2015/08/21 19:04:45, sky wrote: > >> > > > On 2015/08/20 00:04:36, sky wrote: > >> > > > > On 2015/08/20 00:02:57, Nate Chapin wrote: > >> > > > > > On 2015/08/19 23:18:05, sky wrote: > >> > > > > > > On 2015/08/19 22:22:29, Nate Chapin wrote: > >> > > > > > > > FYI, https://codereview.chromium.org/1290053003 was > updated > >> > > > > > > > and > >> > > > > probably(?) > >> > > > > > > > fixes this crash as well. > >> > > > > > > > >> > > > > > > I still get the crash. Here's the stack: > >> > > > > > > > >> > > > > > > > >> > > > > > > > html_viewer.mojo!blink::DocumentLoader::isCommittedButEmpty() > > > > Line > >> > >> > > > 107 C++ > >> > > > > > > html_viewer.mojo!blink::FrameLoader::finishedParsing() > >> > > > > > > Line > >> 485 C++ > >> > > > > > > html_viewer.mojo!blink::FrameLoader::stopAllLoaders() > Line > > > > 980 C++ > >> > >> > > > > > > > >> > html_viewer.mojo!blink::LocalFrame::detach(blink::FrameDetachType > >> > > type) > >> > > > > Line > >> > > > > > > 282 C++ > >> > > > > > > > >> > > > > > > > >> > > > > > > The early out in detach(FrameDetachType type) isn't early > >> > > > > > > enough. > >> > Should > >> > > I > >> > > > > > > proceed with this one? > >> > > > > > > >> > > > > > Do you have repro steps or a test case handy? I'd be > interested > >> > > > > > to > > > > see > >> > >> > > > exactly > >> > > > > > how we're getting into this state. > >> > > > > > >> > > > > Only with mandoline. Build instructions are here: > >> > > > > https://www.chromium.org/developers/mandoline/build (basically > gn > > > > only, > >> > >> > and > >> > > > > *not* component build). Once you build mandoline try displaying > a > >> > > > > page > >> > like > >> > > > > http://amazon.com/ and chances are you'll hit the crash. > >> > > > > It's entirely possible I'm doing something wrong too, not sure. > >> > > > > >> > > > Nate, any update on this? Others are hitting the bug and a bit > >> > > > blocked > > > > by > >> > >> > it. > >> > > > Thanks, > >> > > > > >> > > > -Scott > >> > > > >> > > Sorry, LGTM. There are cleanup opportunities, but I'll keep thinking > >> > > about > >> > them > >> > > rather than blocking you on it. > >> > > >> > BTW, do we have a test? > > > > > >> I don't and am happy to write one if you can point me in the right > >> direction. > >> Are you ok with me landing this and then trying to figure out a test? > > > > > > Yeah, I think that's ok. Given that this looks like it involves detaching > > frames > > inside an event, though, this may be in a gap in our testing coverage. > > Swapping > > frames is probably only possible in a unit test at the moment, but > depending > > on > > JS events to trigger the crash condition strongly implies a layout test. > > > > https://codereview.chromium.org/1298643003/ > To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://src.chromium.org/viewvc/blink?view=rev&revision=201027 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
