Bug 119791

Summary: [Qt] Remove the fix in QWebPage::javaScriptConsoleMessage introduced by (r61433)
Product: WebKit Reporter: Arunprasad Rajkumar <arurajku>
Component: WebKit QtAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: allan.jensen, ararunprasad, commit-queue, hausmann, jturcotte, kenneth, robert
Priority: P3    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 120403    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Arunprasad Rajkumar 2013-08-14 03:26:00 PDT
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.
Comment 1 Arunprasad Rajkumar 2013-08-20 11:19:41 PDT
Created attachment 209210 [details]
Patch
Comment 2 Jocelyn Turcotte 2013-08-21 03:10:52 PDT
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.
Comment 3 Arunprasad Rajkumar 2013-08-21 08:04:21 PDT
Created attachment 209266 [details]
Patch
Comment 4 Jocelyn Turcotte 2013-08-21 09:23:19 PDT
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."
Comment 5 Arunprasad Rajkumar 2013-08-22 12:08:22 PDT
Created attachment 209382 [details]
Patch
Comment 6 Arunprasad Rajkumar 2013-08-22 12:13:02 PDT
(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 7 Jocelyn Turcotte 2013-08-23 02:49:17 PDT
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."?
Comment 8 Arunprasad Rajkumar 2013-08-23 12:15:51 PDT
Created attachment 209490 [details]
Patch
Comment 9 Jocelyn Turcotte 2013-08-26 02:42:17 PDT
Comment on attachment 209490 [details]
Patch

Thanks :)
Comment 10 WebKit Commit Bot 2013-08-26 03:06:22 PDT
Comment on attachment 209490 [details]
Patch

Clearing flags on attachment: 209490

Committed r154593: <http://trac.webkit.org/changeset/154593>
Comment 11 WebKit Commit Bot 2013-08-26 03:06:26 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Allan Sandfeld Jensen 2013-08-28 05:24:01 PDT
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
Comment 13 WebKit Commit Bot 2013-08-28 05:27:31 PDT
Re-opened since this is blocked by bug 120403
Comment 14 Allan Sandfeld Jensen 2013-08-28 05:28:02 PDT
(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.