Bug 119791 – [Qt] Remove the fix in QWebPage::javaScriptConsoleMessage introduced by (r61433)

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Arunprasad Rajkumar]

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.

[Arunprasad Rajkumar]

Created attachment 209210 [details]
Patch

[Jocelyn Turcotte]

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.

[Arunprasad Rajkumar]

Created attachment 209266 [details]
Patch

[Jocelyn Turcotte]

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

[Arunprasad Rajkumar]

Created attachment 209382 [details]
Patch

[Arunprasad Rajkumar]

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

[Jocelyn Turcotte]

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."?

[Arunprasad Rajkumar]

Created attachment 209490 [details]
Patch

[Jocelyn Turcotte]

Comment on attachment 209490 [details]
Patch

Thanks :)

[WebKit Commit Bot]

Comment on attachment 209490 [details]
Patch

Clearing flags on attachment: 209490

Committed r154593: <http://trac.webkit.org/changeset/154593>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Allan Sandfeld Jensen]

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

[WebKit Commit Bot]

Re-opened since this is blocked by bug 120403

[Allan Sandfeld Jensen]

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