975707 - Links in Console load in Console's view on double-click, breaking Console till reopen (if the second click hits the link, not a pop-up)

Status: VERIFIED FIXED

(DevTools :: Console, defect)

Description

[[:Aleksej]]

2014-02-22-03-02-04-mozilla-central-firefox-30.0a1.en-US.linux-x86_64

Steps to reproduce
1. Open Developer Tools → Console.  Optionally disable all buttons but "Net".
2. Load a page which would cause non-short links appear in the Console (e.g. http://ya.ru).  
3. Double-click a link, making sure the second click goes on the link (if a pop-up window appears and interferes, choose the right side of a long link instead).

Actual results:
The link is loaded in the Console view (frame or window), the console is unusable until DevTools are closed and reopened.

Comment 1

[Girish Sharma [:Optimizer]]

I can reproduce it. No need to have long links, just make sure that the mouse location is not above the opened popup.

Mihai, should this be a secure bug ?

Comment 2

[Sami Jaktholm]

Attachment: webconsole-stop-double-click-on-link-from-propagating.patch

A patch that fixes this problem by also stopping double click events on message links from propagating to the default handler.

I'm a but unsure about the test case as it uses setTimeout to wait 500ms for navigation before testing if the frame navigated. I don't know if there's a better way to test for a lack of event or location change but if there is, I'll improve the test case.

Comment 3

[Mihai Sucan [:msucan]]

Comment on attachment 8380301 [details] [diff] [review]
webconsole-stop-double-click-on-link-from-propagating.patch

Review of attachment 8380301 [details] [diff] [review]:
-----------------------------------------------------------------

This is a really good patch. Thank you Sami! Also, thanks to Aleksej for the bug report.

Sami: the test fails here. See https://pastebin.mozilla.org/4379427 I only get this failure when I run all of the webconsole tests. If I run only the test, it passes.

::: browser/devtools/webconsole/test/browser.ini
@@ +225,5 @@
>  [browser_webconsole_bug_817834_add_edited_input_to_history.js]
>  [browser_webconsole_bug_821877_csp_errors.js]
>  [browser_webconsole_bug_837351_securityerrors.js]
>  [browser_webconsole_bug_846918_hsts_invalid-headers.js]
> +[browser_webconsole_bug_975707_dont_navigate_on_doubleclick.js]

Please dont include the bug number in the file name. (we are deprecating bug numbers in file names)

::: browser/devtools/webconsole/test/browser_webconsole_bug_975707_dont_navigate_on_doubleclick.js
@@ +1,4 @@
> +/* vim:set ts=2 sw=2 sts=2 et: */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

You should use the public domain license boilerplate from https://www.mozilla.org/MPL/headers/ (for tests)

@@ +21,5 @@
> +    let messages = yield waitForMessages({
> +      webconsole: hud,
> +      messages: [{
> +        name: "Network request message",
> +        url: TEST_URI

Please also add category: CATEGORY_NETWORK to make sure this is matching what we want.

@@ +32,5 @@
> +    is(hud.iframeWindow.location, CONSOLE_FRAME_URI, "Location before double click is CONSOLE_FRAME_URI.");
> +    EventUtils.synthesizeMouseAtCenter(urlNode, {clickCount: 2}, hud.iframeWindow);
> +
> +    let deferred = promise.defer();
> +    setTimeout(deferred.resolve, 500);

Instead of the timeout you could:

- wait for the click event and check if preventDefault() was called (event.defaultPrevented).
- wait for domcontentloaded/load (check which one is fired when your fix is not applied). in the event handler you can do ok(false, "console iframe window navigated").

Comment 4

[Mihai Sucan [:msucan]]

(In reply to Girish Sharma [:Optimizer] from comment #1)
> I can reproduce it. No need to have long links, just make sure that the
> mouse location is not above the opened popup.
> 
> Mihai, should this be a secure bug ?

Not sure. Does this allow privilege escalation?

Comment 5

[Girish Sharma [:Optimizer]]

(In reply to Mihai Sucan [:msucan] from comment #4)
> (In reply to Girish Sharma [:Optimizer] from comment #1)
> > I can reproduce it. No need to have long links, just make sure that the
> > mouse location is not above the opened popup.
> > 
> > Mihai, should this be a secure bug ?
> 
> Not sure. Does this allow privilege escalation?

Not sure either :)

Comment 6

[Sami Jaktholm]

Attachment: webconsole-stop-double-click-on-link-from-propagating-v2.patch

Thanks for the review. Here's a new version that should fix the issues you pointed out.

For the test I chose to check that once click event arrives, the default action is prevented. Apparently when all tests were run the page is cached and thus doesn't show up in the console at all. This was causing the test to fail earlier...

Comment 7

[Mihai Sucan [:msucan]]

Comment on attachment 8380628 [details] [diff] [review]
webconsole-stop-double-click-on-link-from-propagating-v2.patch

Review of attachment 8380628 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good! Thanks for the quick update. Tests pass now.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=189cb0884245

Comment 8

[Mihai Sucan [:msucan]]

Try push seems green enough.

Landed: https://hg.mozilla.org/integration/fx-team/rev/70f928fe9ecf

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/70f928fe9ecf

Comment 10

[[:Aleksej]]

Verified fixed with 2014-03-09-03-02-04-mozilla-central-firefox-30.0a1.en-US.linux-x86_64: double click causes selection of the word.  Thanks.