There is fix introduced by http://trac.webkit.org/r61433 in QWebPage::javaScriptConsoleMessage to handle the derived class destruction situation(only for DRT run). Once https://bugs.webkit.org/show_bug.cgi?id=119654 is landed we are free from that issue & we can remove the fix.
Created attachment 209210 [details] Patch
Comment on attachment 209210 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=209210&action=review > Source/WebKit/qt/ChangeLog:9 > + DRT introduced by http://trac.webkit.org/r61433. We usually just write r61433 without the full URL in ChangeLogs. > Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:164 > + // Load empty url to send onunload event to currently running page. > + // onunload event is mandatory for LayoutTests/plugins/open-and-close-window-with-plugin.html and > + // LayoutTests/plugins/geturlnotify-during-document-teardown.html. Please mention that this has to do with the virtual table still being intact at this point.
Created attachment 209266 [details] Patch
Comment on attachment 209266 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=209266&action=review > Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:166 > + // lead(eg: calling window.alert from window.onunload) to call the > + // virtual functions of the partially deleted WebPage object. > + // At this point vptr of QWebPage points to the derived class WebPage. This is enough information but still too confusing: - "from ~QWebPage will lead" -> "from ~QWebPage would lead". You want to avoid it. - It won't "call the virtual functions of the partially deleted WebPage object" in ~QWebPage, it will call virtual functions of QWebPage as the WebPage part of the vtable has already been unwinded. - "At this point vptr of QWebPage points to the derived class WebPage.", at which point, I assume that you mean here in ~WebPage? Please clarify. - "vptr of QWebPage" -> "the vtable of QWebPage" - "eg" -> "e.g."
Created attachment 209382 [details] Patch
(In reply to comment #5) > Created an attachment (id=209382) [details] > Patch Some of the LayoutTests for "http" were failing regardless of this fix. May be my Layout Test setup issue.
Comment on attachment 209382 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=209382&action=review > Source/WebKit/qt/ChangeLog:3 > + <https://webkit.org/b/119791> [Qt] Remove the fix in QWebPage::javaScriptConsoleMessage introduced by (r61433) There has been some discussion on webkit-dev that we should avoid the new changelog format of prepare-changelog and edit it manually to use the old https://bugs.webkit.org/show_bug.cgi?id=119791 URL format on the second line instead. The rests are mostly nitpicks, the comment could still use a bit of polishing: > Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:162 > + // Load an empty url to send onunload event to the running page before *the* onunload event > Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:164 > + // Prior to this fix onunload event would be triggered from '~QWebPage', but *the* onunload event > Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:165 > + // it may call virtual functions(e.g. calling a window.alert from window.onunload) space before the parenthesis > Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:167 > + // When control is in '~WebPage' the vtable of 'QWebPage' points to the derived "When *the* control is in '~WebPage'" or just "When in '~WebPage'" > Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:168 > + // class 'WebPage' and safe to send onunload event. It was safe to send it, but it wasn't possible to receive virtual calls from the JS handler. Maybe something like: "and it's possible to receive javaScriptAlert calls."?
Created attachment 209490 [details] Patch
Comment on attachment 209490 [details] Patch Thanks :)
Comment on attachment 209490 [details] Patch Clearing flags on attachment: 209490 Committed r154593: <http://trac.webkit.org/changeset/154593>
All reviewed patches have been landed. Closing bug.
This patch caused an additional 50+ tests to become flaky with Qt 5.1 on WebKit1 http://build.webkit.sed.hu/builders/x86-64 Linux Qt Release - Qt5.1 - WebKit1?numbuilds=100
Re-opened since this is blocked by bug 120403
(In reply to comment #12) > This patch caused an additional 50+ tests to become flaky with Qt 5.1 on WebKit1 http://build.webkit.sed.hu/builders/x86-64 Linux Qt Release - Qt5.1 - WebKit1?numbuilds=100 Same on Qt 5.0, rolling out.