1311477 - Avoid using an unsafe CPOW in browser_save_link-perwindowpb.js

Status: RESOLVED FIXED

(Firefox :: Private Browsing, defect)

Comment 1

[(no longer active)]

Attachment: Avoid using an unsafe CPOW in browser_save_link-perwindowpb.js

This patch doesn't work in its current form.  I don't get any debug output anywhere, and I haven't been able to find a printf like function that works within a dispatched ContentTask, so I'm not sure what to look into next.

Blake, can you please have a look and see if you can think of why this wouldn't work?

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8802626 [details] [diff] [review]
Avoid using an unsafe CPOW in browser_save_link-perwindowpb.js

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

::: browser/base/content/test/general/browser_save_link-perwindowpb.js
@@ +12,5 @@
>    info("started triggerSave");
>    var fileName;
>    let testBrowser = aWindow.gBrowser.selectedBrowser;
>    // This page sets a cookie if and only if a cookie does not exist yet
> +  let testURI = getRootDirectory(gTestPath) + "bug792517-2.html";

I'm pretty sure gTestPath is a chrome: URI, which could be why this doesn't seem to do anything. It might be easier to leave this line alone.

@@ +17,2 @@
>    testBrowser.loadURI(testURI);
>    testBrowser.addEventListener("pageshow", function pageShown(event) {

Is there a reason to use pageshow here? It uses a shim. It seems like we could just use

BrowserTestUtils.browserLoaded(testBrowser, testURI).then(() => { ... });

Alternatively, if we have to use pageshow, we could even replace it with

ContentTask.spawn(testBrowser, testURI, function* (testURI) {
  yield ContentTaskUtils.waitForEvent(this, "pageshow", true, (event) => event.target.location == testURI);
}).then(() => { ... });

@@ +33,5 @@
> +        var EventUtils = {};
> +        Services.scriptloader.loadSubScript(
> +          "chrome://mochikit/content/tests/SimpleTest/EventUtils.js", EventUtils);
> +        var link = content.document.getElementById(id);
> +        EventUtils.synthesizeMouseAtCenter(link,

This can also be done more easily, BrowserTestUtils has just the thing to do this without rolling your own EventUtils:

BrowserTestUtils.synthesizeMouseAtCenter("fff", { type: "contextmenu", button: 2 }, testBrowser);

(I haven't tested this, but it should work.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Oops, I meant to type synthesizeMouseAtCenter("#fff", ...) (it uses document.querySelector in the content process).

Comment 4

[(no longer active)]

Attachment: Avoid using an unsafe CPOW in browser_save_link-perwindowpb.js

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8803115 [details] [diff] [review]
Avoid using an unsafe CPOW in browser_save_link-perwindowpb.js

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

Thanks!

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/14becb61ce3c
Avoid using an unsafe CPOW in browser_save_link-perwindowpb.js; r=mrbkap

Comment 7

[Carsten Book [:Tomcat]]

sorry backed this out for test failures in bc7 tests like https://treeherder.mozilla.org/logviewer.html#?job_id=38026535&repo=mozilla-inbound

Comment 8

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a176dc5a703
Backed out changeset 14becb61ce3c

Comment 9

[(no longer active)]

As far as I can tell, my patch is supposedly fixing bug 1297593, but I was backed out because of bug 1204199, which is, well, ironic.  :-)

Comment 11

[(no longer active)]

(I tried retriggering a test but trees are closed due to the dyn.com issue.)

Comment 12

[(no longer active)]

This was just a stupid bug in my patch.  I forgot to pass testURI as the third arg to browserLoaded, so when the test opened a new private window, sometimes it would end up proceeding with only about:privatebrowsing loaded in the PB window.

Fixing this gives me a push that's fairly green on try: <https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd966dfd77c3b735e32ce507830ec35ffe0db55a>

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/621f85162b23
Avoid using an unsafe CPOW in browser_save_link-perwindowpb.js; r=mrbkap

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/621f85162b23

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/982c72148922

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/bce0b2a3bd3f