When a website opens a new window inside a beforeunload event, Safari leaves the window open because we prevent the initial navigation. <rdar://problem/32934449>
Created attachment 314247 [details] Fixes the bug
Created attachment 314248 [details] Updated the change log
Comment on attachment 314248 [details] Updated the change log View in context: https://bugs.webkit.org/attachment.cgi?id=314248&action=review > Source/WebCore/ChangeLog:32 > + the frame is null (durign the destruction of a frameless document) in which case we increment the global typo: during
Committed r219008: <http://trac.webkit.org/changeset/219008>
Comment on attachment 314248 [details] Updated the change log View in context: https://bugs.webkit.org/attachment.cgi?id=314248&action=review >> Source/WebCore/ChangeLog:32 >> + the frame is null (durign the destruction of a frameless document) in which case we increment the global > > typo: during Nit: Here you use "frameless", but above you use "frame-less". > Source/WebCore/ChangeLog:36 > + document in destruction, and none of the frame in the same frame tree as the one given is currently in I suggest using the phrase "being destroyed" instead of "in destruction" to make these sentence easier to read Nit: "the frame" => "the frames" (notice the pluralization) > Source/WebCore/WebCore.xcodeproj/project.pbxproj:23989 > + 9BA827781F06156500F71E75 /* NavigationDisabler.h */, Please add this file so that it is in sorted order with the other files in this list. > Source/WebCore/dom/Document.cpp:2300 > + NavigationDisabler navigationDisabler(m_frame); Nit: Although an unwritten convention there is a moment to use C++11 brace initialization in new code. > Source/WebCore/loader/FrameLoader.cpp:2921 > + NavigationDisabler navigationDisabler(&m_frame); Ditto. > Source/WebCore/loader/NavigationDisabler.h:38 > + m_frame->mainFrame().m_navigationDisableCount++; Are you planning to replace SubframeLoadingDisabler with this class? If not, would it make sense to have m_navigationDisableCount live on Page? Nit: This is OK as-is. Although the compiler is likely smart enough to do this for us, I suggest using frame instead of m_frame here. Nit: This is OK as-is. The compiler is smart enough to know that we are not using the return value of this post-increment operator and convert to using a pre-increment. For your consideration, I suggest writing this using a pre-increment for consistency with the generated code. > Source/WebCore/loader/NavigationDisabler.h:40 > + s_globalNavigationDisableCount++; Ditto. > Source/WebCore/loader/NavigationDisabler.h:48 > + mainFrame.m_navigationDisableCount--; Nit: This is OK as-is. For your consideration, I suggest writing this using a pre-decrement for consistency with the generated code by the same argument as above > Source/WebCore/loader/NavigationDisabler.h:51 > + s_globalNavigationDisableCount--; Ditto.
Comment on attachment 314248 [details] Updated the change log View in context: https://bugs.webkit.org/attachment.cgi?id=314248&action=review > Source/WebCore/ChangeLog:4 > + https://bugs.webkit.org/show_bug.cgi?id=174016 I know that this patch was already reviewed and landed. For future reference, we tend to put the radar bug URL under the WebKit build URL.
(In reply to Daniel Bates from comment #5) > Comment on attachment 314248 [details] > Updated the change log > > View in context: > https://bugs.webkit.org/attachment.cgi?id=314248&action=review > > > Source/WebCore/ChangeLog:36 > > + document in destruction, and none of the frame in the same frame tree as the one given is currently in > > I suggest using the phrase "being destroyed" instead of "in destruction" to > make these sentence easier to read > > Nit: "the frame" => "the frames" (notice the pluralization) > > > Source/WebCore/WebCore.xcodeproj/project.pbxproj:23989 > > + 9BA827781F06156500F71E75 /* NavigationDisabler.h */, > > Please add this file so that it is in sorted order with the other files in > this list. Maybe we should just run ./Tools/Scripts/sort-Xcode-project-file Source/WebCore/WebCore.xcodeproj/ automatically in webkit-patch upload. It's really silly that we ask people to do it themselves. Anyhow, none of this will matter in a few months once we switch to CMake builds. > > Source/WebCore/dom/Document.cpp:2300 > > + NavigationDisabler navigationDisabler(m_frame); > > Nit: Although an unwritten convention there is a moment to use C++11 brace > initialization in new code. I don't think C++11 brace initialization is any better in this case since there is exactly one argument. It doesn't improve anything. > > Source/WebCore/loader/FrameLoader.cpp:2921 > > + NavigationDisabler navigationDisabler(&m_frame); > > Ditto. Ditto. > > Source/WebCore/loader/NavigationDisabler.h:38 > > + m_frame->mainFrame().m_navigationDisableCount++; > > Are you planning to replace SubframeLoadingDisabler with this class? If not, > would it make sense to have m_navigationDisableCount live on Page? No. Turns out that SubframeLoadingDisabler is used to disable loading of a frame inside a particular DOM tree within a specific document, not across frames. So it's a fundamentally different class than NavigationDisabler which tries to disable navigations across frames. We can't use Page here because Page isn't a ref-counted object, and Frame can have a null Page. It's simply not safe to access Page via Frame.
Sorted xcodeproject files in https://trac.webkit.org/changeset/219009.
Committed r219039: <http://trac.webkit.org/changeset/219039>