Last Comment Bug 724161 - simplify drag&drop checks for javascript:/data: URIs
: simplify drag&drop checks for javascript:/data: URIs
Status: RESOLVED FIXED
[qa-]
:
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 13
Assigned To: :Gavin Sharp [email: gavin@gavinsharp.com]
:
Mentors:
Depends on: 718203
Blocks: CVE-2012-0455
  Show dependency treegraph
 
Reported: 2012-02-03 16:33 PST by :Gavin Sharp [email: gavin@gavinsharp.com]
Modified: 2012-02-29 11:49 PST (History)
2 users (show)
gavin.sharp: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed
fixed


Attachments
patch (2.51 KB, patch)
2012-02-03 16:36 PST, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details | Diff | Splinter Review
patch (8.40 KB, patch)
2012-02-14 15:09 PST, :Gavin Sharp [email: gavin@gavinsharp.com]
dao+bmo: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
lukasblakk+bugs: approval‑mozilla‑esr10+
Details | Diff | Splinter Review

Description :Gavin Sharp [email: gavin@gavinsharp.com] 2012-02-03 16:33:55 PST
The patch for bug 718203 exposes a nicer API for filtering out javascript:/data: link drops, we should make use of that rather than having different string-comparison checks.
Comment 1 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-02-03 16:36:45 PST
Created attachment 594359 [details] [diff] [review]
patch
Comment 2 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-02-14 15:09:35 PST
Created attachment 597203 [details] [diff] [review]
patch

with tests
Comment 3 Dão Gottwald [:dao] 2012-02-15 04:17:26 PST
Comment on attachment 597203 [details] [diff] [review]
patch

>+++ b/browser/base/content/test/browser_tabDrop.js

>+      chromeUtils.synthesizeDrop(newTab, newTab, [[{type: "text/plain", data: text}]], "link", window, EventUtils);

I would expect this to load the links in the current tab, but you're expecting new tabs. What's going on here?
Comment 4 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-02-15 10:32:01 PST
(In reply to Dão Gottwald [:dao] from comment #3)
> I would expect this to load the links in the current tab, but you're
> expecting new tabs. What's going on here?

Good catch. The synthesizeDrop code is producing events with screenX/screenY equal to 0, which makes _getDragTargetTab return null.

Fixing synthesizeDrop to pass realistic coordinates seems like it might be annoying. What do you think about just adding a comment explaining the reliance on the bug?
Comment 5 Dão Gottwald [:dao] 2012-02-16 05:56:02 PST
Could the synthetic drop target the left or right side of the tab so that it keeps working in case that bug gets fixed?
Comment 6 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-02-16 13:18:09 PST
(In reply to Dão Gottwald [:dao] from comment #5)
> Could the synthetic drop target the left or right side of the tab so that it
> keeps working in case that bug gets fixed?

There's no way to control the specific coordinates of the drop. I think anyone fixing synthesizeDrop to pass non-bogus coordinates will also have to add support for specifying custom offsets to avoid breaking the world, so it makes more sense to just deal with this test when (if) that happens.
Comment 7 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-02-17 14:42:00 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/50aae34555bb
Comment 8 Ed Morley [:emorley] 2012-02-18 09:57:39 PST
https://hg.mozilla.org/mozilla-central/rev/50aae34555bb
Comment 9 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-02-18 18:27:18 PST
Comment on attachment 597203 [details] [diff] [review]
patch

[Approval Request Comment]
fix for bug 704354. depends on patch for bug 718203.
Comment 10 Alex Keybl [:akeybl] 2012-02-21 08:57:21 PST
Comment on attachment 597203 [details] [diff] [review]
patch

[Triage Comment]
Approving for Aurora/Beta in support of bug 704354. Please land today.
Comment 12 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-02-29 11:49:06 PST
https://hg.mozilla.org/releases/mozilla-esr10/rev/566be745f397

Note You need to log in before you can comment on or make changes to this bug.