Bug 174004 – Frame.h doesn't need to include FrameLoader.h, IntRect.h, and NavigationScheduler.h

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Ryosuke Niwa]

Remove these header dependencies.

[Ryosuke Niwa]

Created attachment 314208 [details]
Cleanup

[Ryosuke Niwa]

Created attachment 314212 [details]
WPE build fix attempt

[Ryosuke Niwa]

Created attachment 314213 [details]
Another WPE build fix attempt

[Ryosuke Niwa]

I know this builds on Mac & OS.

[Build Bot]

Comment on attachment 314213 [details]
Another WPE build fix attempt

Attachment 314213 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4023677

New failing tests:
imported/w3c/web-platform-tests/html/browsers/windows/browsing-context-names/choose-_self-001.html
http/tests/security/xss-DENIED-xsl-document-securityOrigin.xml
imported/w3c/web-platform-tests/html/browsers/windows/browsing-context-names/browsing-context-choose-self-1.html
fast/dom/Window/window-open-self-from-other-frame.html
http/tests/websocket/construct-in-detached-frame.html
imported/w3c/web-platform-tests/html/browsers/windows/browsing-context-names/choose-_self-002.html

[Build Bot]

Created attachment 314220 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Ryosuke Niwa]

Oh, some tests are crashing. WTF!?

[Build Bot]

Comment on attachment 314213 [details]
Another WPE build fix attempt

Attachment 314213 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4023766

New failing tests:
http/tests/websocket/construct-in-detached-frame.html
imported/w3c/web-platform-tests/html/browsers/windows/browsing-context-names/browsing-context-choose-self-2.html

[Build Bot]

Created attachment 314224 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 314213 [details]
Another WPE build fix attempt

Attachment 314213 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4023691

New failing tests:
imported/w3c/web-platform-tests/html/browsers/windows/browsing-context-names/choose-_self-002.html
imported/w3c/web-platform-tests/html/browsers/windows/browsing-context-names/browsing-context-choose-self-2.html
http/tests/websocket/construct-in-detached-frame.html
http/tests/security/xss-DENIED-xsl-document.xml
fast/dom/Window/window-open-self.html

[Build Bot]

Created attachment 314225 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Build Bot]

Comment on attachment 314213 [details]
Another WPE build fix attempt

Attachment 314213 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4023914

New failing tests:
inspector/dom/hideHighlight.html
inspector/dom/highlightSelector.html
imported/w3c/web-platform-tests/html/browsers/windows/browsing-context-names/choose-_self-001.html
http/tests/security/xss-DENIED-xsl-document-securityOrigin.xml
inspector/dom/highlightRect.html
inspector/dom/highlight-shape-outside-margin.html
fast/dom/Window/window-open-self-from-other-frame.html
http/tests/websocket/construct-in-detached-frame.html
inspector/dom/highlight-flow-with-no-region.html
inspector/dom/highlightFrame.html
imported/w3c/web-platform-tests/html/browsers/windows/browsing-context-names/choose-_self-002.html
inspector/dom/highlightNode.html
inspector/dom/highlightQuad.html
imported/w3c/web-platform-tests/html/browsers/windows/browsing-context-names/browsing-context-choose-self-1.html

[Build Bot]

Created attachment 314232 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 314213 [details]
Another WPE build fix attempt

Attachment 314213 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4024481

New failing tests:
inspector/dom/hideHighlight.html
inspector/dom/highlightSelector.html
imported/w3c/web-platform-tests/html/browsers/windows/browsing-context-names/choose-_self-001.html
http/tests/security/xss-DENIED-xsl-document-securityOrigin.xml
inspector/dom/highlightRect.html
inspector/dom/highlight-shape-outside-margin.html
fast/dom/Window/window-open-self-from-other-frame.html
http/tests/websocket/construct-in-detached-frame.html
inspector/dom/highlight-flow-with-no-region.html
inspector/dom/highlightFrame.html
imported/w3c/web-platform-tests/html/browsers/windows/browsing-context-names/choose-_self-002.html
inspector/dom/highlightNode.html
inspector/dom/highlightQuad.html
imported/w3c/web-platform-tests/html/browsers/windows/browsing-context-names/browsing-context-choose-self-1.html

[Build Bot]

Created attachment 314241 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Ryosuke Niwa]

Created attachment 314250 [details]
Fixed the tests

[Ryosuke Niwa]

Created attachment 314251 [details]
Fixed the tests

[Build Bot]

Comment on attachment 314251 [details]
Fixed the tests

Attachment 314251 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4025594

New failing tests:
inspector/dom/hideHighlight.html
inspector/dom/highlightSelector.html
inspector/dom/highlightRect.html
inspector/dom/highlight-shape-outside-margin.html
inspector/dom/highlightFrame.html
inspector/dom/highlightNode.html
inspector/dom/highlightQuad.html
inspector/dom/highlight-flow-with-no-region.html

[Build Bot]

Created attachment 314255 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Simon Fraser (smfr)]

Comment on attachment 314251 [details]
Fixed the tests

View in context: https://bugs.webkit.org/attachment.cgi?id=314251&action=review

> Source/WebKit/win/WebCoreSupport/WebFrameLoaderClient.cpp:992
> +    core(m_webFrame)->createView(enclosingIntRect(logicalFrame).size(), backgroundColor, transparent, /* fixedLayoutSize */ IntSize(), /* fixedVisibleContentRect */IntRect());

Can't the default args be "{ }"?

[Ryosuke Niwa]

Created attachment 314309 [details]
Patch for landing

[Ryosuke Niwa]

Comment on attachment 314309 [details]
Patch for landing

Wait for EWS.

[Ryosuke Niwa]

Created attachment 314317 [details]
Patch for landing

[Ryosuke Niwa]

Comment on attachment 314309 [details]
Patch for landing

Oops, I forgot to resolve conflicts.

[Ryosuke Niwa]

Comment on attachment 314317 [details]
Patch for landing

Wait for EWS.

[Build Bot]

Comment on attachment 314317 [details]
Patch for landing

Attachment 314317 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4030300

New failing tests:
fast/workers/worker-exception-during-navigation.html

[Build Bot]

Created attachment 314345 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Ryosuke Niwa]

(In reply to Build Bot from comment #26)
> Comment on attachment 314317 [details]
> Patch for landing
> 
> Attachment 314317 [details] did not pass mac-wk2-ews (mac-wk2):
> Output: http://webkit-queues.webkit.org/results/4030300
> 
> New failing tests:
> fast/workers/worker-exception-during-navigation.html

I don't think this crash is related to my patch:
Thread 45 Crashed:: WTF::AutomaticThread
0   com.apple.JavaScriptCore      	0x0000000108ae2dba WTF::ScopedLambdaRefFunctor<void (__darwin_x86_thread_state64&), JSC::VMTraps::SignalSender::work()::'lambda'(__darwin_x86_thread_state64&)>::implFunction(void*, __darwin_x86_thread_state64&) + 58 (JSLock.h:97)
1   com.apple.JavaScriptCore      	0x0000000108bd6c85 WTF::sendMessageScoped(WTF::Thread&, WTF::ScopedLambda<void (__darwin_x86_thread_state64&)> const&) + 85 (ThreadMessage.cpp:48)
2   com.apple.JavaScriptCore      	0x0000000108ae2b81 JSC::VMTraps::SignalSender::work() + 97 (ThreadMessage.h:49)
3   com.apple.JavaScriptCore      	0x0000000108b9edc0 WTF::Function<void ()>::CallableWrapper<WTF::AutomaticThread::start(WTF::AbstractLocker const&)::$_0>::call() + 240 (AutomaticThread.cpp:217)
4   com.apple.JavaScriptCore      	0x0000000108bd61df WTF::threadEntryPoint(void*) + 111 (memory:2459)
5   com.apple.JavaScriptCore      	0x0000000108bd660a WTF::wtfThreadEntryPoint(void*) + 74 (utility:765)
6   libsystem_pthread.dylib       	0x00007fff8745599d _pthread_body + 131
7   libsystem_pthread.dylib       	0x00007fff8745591a _pthread_start + 168
8   libsystem_pthread.dylib       	0x00007fff87453351 thread_start + 13

[Ryosuke Niwa]

Comment on attachment 314317 [details]
Patch for landing

Clearing flags on attachment: 314317

Committed r219051: <http://trac.webkit.org/changeset/219051>

[Ryosuke Niwa]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

These always could have been pointers. Originally they weren’t because we thought there was a performance benefit to not dereferencing a pointer or using a separate memory block. That seems to be what we should be discussing here. Is it OK to pay the extra overhead of a separate memory object for these two objects? Presumably the answer is yes, but I am not sure they go without saying.

[Ryosuke Niwa]

(In reply to Darin Adler from comment #31)
> These always could have been pointers. Originally they weren’t because we
> thought there was a performance benefit to not dereferencing a pointer or
> using a separate memory block. That seems to be what we should be discussing
> here. Is it OK to pay the extra overhead of a separate memory object for
> these two objects? Presumably the answer is yes, but I am not sure they go
> without saying.

Frame is a rare enough object and the code that touches FrameLoader or NavigationScheduler are rare enough & not hot enough that this shouldn't matter but we should probably come up with some strategy to avoid allocating memory each time these member variables are allocated.

For example, we could come up with a mechanism to allocate memory for all these UniqueRef members at once instead of allocating for each one.

[Darin Adler]

We don’t need to do anything fancy if there is no real problem. I was never concerned about the extra memory overhead of allocating a couple more malloc blocks, but I thought that adding to a pointer might be more efficient than dereferencing a pointer to find the frame loader given the frame.

If we’re sure that’s not significant then lets not worry about it.