Bug 152690 – [GTK] JavaScript prompt uses title of page to be loaded rather than title of current page

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Michael Catanzaro]

Created attachment 268193 [details]
screenshot

When viewing an internal page with some forms, I decided to switch to the WebKit bot watcher dashboard. The attached dialog popped up when I tried to change the page. The URL in the title bar is messed up -- it's a prompt related to the page I was currently viewing, not the bot watcher dashboard that I was trying to switch to, so it it showing the wrong URL.

(Unrelated: this dialog looks like shit thanks to GTK+ 3.19.)

[Claudio Saavedra]

Created attachment 309623 [details]
Patch

[Claudio Saavedra]

Comment on attachment 309623 [details]
Patch

I confess I didn't test this.

[Claudio Saavedra]

Well okay, I did test it but only with a window.alert(), but it should fix this bug.

[Build Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Michael Catanzaro]

Comment on attachment 309623 [details]
Patch

The change looks correct. One nit: I'm not super comfortable with using the current title without having tested an actual beforeunload prompt. So I would change the title of this bug, and the title in the changelog to match. I think you can just remove the word "beforeunload" from the title: "JavaScript prompt uses title of page to be loaded rather than title of current page." (Oh look, I forgot a "than" in the original title!)

[Claudio Saavedra]

Created attachment 309695 [details]
Patch for landing

[Carlos Garcia Campos]

(In reply to Claudio Saavedra from comment #3)
> Well okay, I did test it but only with a window.alert(), but it should fix
> this bug.

No, beforeunload are different to alert, that's why this bug only happens with beforeunload. Unless you tried an alert called from an unload event handler. In case of alerts, the prompts nomrally happen when the load has already been committed, so webkit_web_view_get_uri() returns the committed URL. Same for all other prompts, unless they are called from page unload, of course. But beforeunload events can be called when a new navigation starts and the previous page is going to be replaced by a new one. In this case webkit_web_get_uri() returns the pending API request URL, but we want the committed URL in this case.

[Claudio Saavedra]

(In reply to Carlos Garcia Campos from comment #7)
> (In reply to Claudio Saavedra from comment #3)
> > Well okay, I did test it but only with a window.alert(), but it should fix
> > this bug.
> 
> No, beforeunload are different to alert, that's why this bug only happens
> with beforeunload. Unless you tried an alert called from an unload event
> handler. In case of alerts, the prompts nomrally happen when the load has
> already been committed, so webkit_web_view_get_uri() returns the committed
> URL. Same for all other prompts, unless they are called from page unload, of
> course. But beforeunload events can be called when a new navigation starts
> and the previous page is going to be replaced by a new one. In this case
> webkit_web_get_uri() returns the pending API request URL, but we want the
> committed URL in this case.

But using the committed URL always is safe for all four different prompts, no? That's my point and that's what this patch is doing.

[WebKit Commit Bot]

Comment on attachment 309695 [details]
Patch for landing

Clearing flags on attachment: 309695

Committed r216670: <http://trac.webkit.org/changeset/216670>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Carlos Garcia Campos]

(In reply to Claudio Saavedra from comment #8)
> (In reply to Carlos Garcia Campos from comment #7)
> > (In reply to Claudio Saavedra from comment #3)
> > > Well okay, I did test it but only with a window.alert(), but it should fix
> > > this bug.
> > 
> > No, beforeunload are different to alert, that's why this bug only happens
> > with beforeunload. Unless you tried an alert called from an unload event
> > handler. In case of alerts, the prompts nomrally happen when the load has
> > already been committed, so webkit_web_view_get_uri() returns the committed
> > URL. Same for all other prompts, unless they are called from page unload, of
> > course. But beforeunload events can be called when a new navigation starts
> > and the previous page is going to be replaced by a new one. In this case
> > webkit_web_get_uri() returns the pending API request URL, but we want the
> > committed URL in this case.
> 
> But using the committed URL always is safe for all four different prompts,
> no? That's my point and that's what this patch is doing.

Yes, I haven't said the patch is wrong, only that testing with an alert doesn't help with this bug. And that the actual cause should be explained in the ChangeLog instead of current text that is not accurate:

"webkit_web_view_get_uri() returns the page to be loaded, use internal api for this."

webkit_web_view_get_uri() returns page to be loaded right after a navigation and before the provisional load, but only in that case, that's why this bug was never noticed with other prompts, webkit_web_view_get_uri() returns the committed URL when the load has been committed. What you call the internal API is not accurate either, becaus there's internal API to get the active URL, the provisional URL, the committed URL and unreachable URL. What the patch dos now  is not using internal API, but always getting the committed URL.