Bug 174016 – REGRESSION(r214194): Safari leaves a popup window open opened during before unload

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Ryosuke Niwa]

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>

[Ryosuke Niwa]

Created attachment 314247 [details]
Fixes the bug

[Ryosuke Niwa]

Created attachment 314248 [details]
Updated the change log

[Chris Dumez]

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

[Ryosuke Niwa]

Committed r219008: <http://trac.webkit.org/changeset/219008>

[Daniel Bates]

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.

[Daniel Bates]

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.

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

Sorted xcodeproject files in https://trac.webkit.org/changeset/219009.

[Ryosuke Niwa]

Committed r219039: <http://trac.webkit.org/changeset/219039>