Closed Bug 1311477 Opened 7 years ago Closed 7 years ago

Avoid using an unsafe CPOW in browser_save_link-perwindowpb.js


(Firefox :: Private Browsing, defect)

Not set



Firefox 52
Tracking Status
firefox50 --- fixed
firefox51 --- fixed
firefox52 --- fixed


(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)




(1 file, 1 obsolete file)

      No description provided.
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?
Attachment #8802626 - Flags: feedback?(mrbkap)
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) => == 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.
Attachment #8802626 - Flags: feedback?(mrbkap) → feedback-
Oops, I meant to type synthesizeMouseAtCenter("#fff", ...) (it uses document.querySelector in the content process).
Attachment #8802626 - Attachment is obsolete: true
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]:

Attachment #8803115 - Flags: review?(mrbkap) → review+
Pushed by
Avoid using an unsafe CPOW in browser_save_link-perwindowpb.js; r=mrbkap
sorry backed this out for test failures in bc7 tests like
Flags: needinfo?(ehsan)
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.  :-)
(I tried retriggering a test but trees are closed due to the issue.)
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: <>
Flags: needinfo?(ehsan)
Pushed by
Avoid using an unsafe CPOW in browser_save_link-perwindowpb.js; r=mrbkap
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Assignee: nobody → ehsan
You need to log in before you can comment on or make changes to this bug.