778076 - mochitest-plain 1852 TEST-UNEXPECTED-FAIL | /tests/suite/browser/test/test_contextmenu.html | checking item #0 (popupwindow-reject) name - got popupwindow-allow, expected popupwindow-reject

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[Philip Chee]

mochitest-plain 1852 TEST-UNEXPECTED-FAIL | /tests/suite/browser/test/test_contextmenu.html | checking item #0 (popupwindow-reject) name - got popupwindow-allow, expected popupwindow-reject

See http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1343366654.1343367158.15908.gz

$ MOZ_NO_REMOTE=1 TEST_PATH=suite/browser/test/test_contextmenu.html  pymake -C ../objdir-sm/ mochitest-plain

Passed: 1693
Failed: 20
Todo: 0

failed | checking item #0 (popupwindow-reject) name - got popupwindow-allow, expected popupwindow-reject
failed | checking item #0 (popupwindow-reject) name - got popupwindow-allow, expected popupwindow-reject
failed | checking item #0 (popupwindow-reject) name - got popupwindow-allow, expected popupwindow-reject
failed | checking item #0 (popupwindow-reject) name - got popupwindow-allow, expected popupwindow-reject
failed | checking item #0 (popupwindow-reject) name - got popupwindow-allow, expected popupwindow-reject
failed | checking item #0 (popupwindow-reject) name - got popupwindow-allow, expected popupwindow-reject
failed | checking item #0 (popupwindow-reject) name - got popupwindow-allow, expected popupwindow-reject
failed | checking item #0 (popupwindow-reject) name - got popupwindow-allow, expected popupwindow-reject
failed | checking item #0 (popupwindow-reject) name - got popupwindow-allow, expected popupwindow-reject
failed | checking item #0 (popupwindow-reject) name - got popupwindow-allow, expected popupwindow-reject
failed | checking item #0 (popupwindow-reject) name - got popupwindow-allow, expected popupwindow-reject
failed | checking item #0 (popupwindow-reject) name - got popupwindow-allow, expected popupwindow-reject
failed | checking item #0 (popupwindow-reject) name - got popupwindow-allow, expected popupwindow-reject
failed | checking item #0 (popupwindow-reject) name - got popupwindow-allow, expected popupwindow-reject

nsContextMenu.js:

>     var blocking = true;
>       if (this.popupURL)
>         try {
>           const PM = Components.classes["@mozilla.org/PopupWindowManager;1"]
>                      .getService(Components.interfaces.nsIPopupWindowManager);
>           blocking = PM.testPermission(this.popupURL) ==
>                      Components.interfaces.nsIPopupWindowManager.DENY_POPUP;
>         } catch (e) {
>         }

Unfortunately the error is swallowed by the try/catch block :P

Comment 1

[Philip Chee]

Attachment: Patch v1.0 Proposed Fix.

Comment 2

[Philip Chee]

Attachment: Patch v1.0 Proposed Fix.

Fallout from Bug 769586 - Make PopupWindowManager using principal to test permissions instead of URI.

In addition since 2003 Bug 191380 - Rewrite permission code (for cookies, images etc)
There is no add() method in nsIPopupWindowManager !!!!!!!

Our popup blocking code seems to date back to 2002-09-28 Bug 166442. Erk!

This patch fixes the test errors:

Passed: 1713
Failed: 0
Todo: 0

To test the popup blocking code in the context menu:
1. Set dom.disable_open_during_load to FLASE
2. Visit http://www.pogo.com/misc/popup-blocker-test.jsp

Comment 3

[[e-mail]]

Comment on attachment 646510 [details]
Patch v1.0 Proposed Fix.

># User Neil Rashbrook <[e-mail]>
Eek! Imposter Alert!

>-          const PM = Components.classes["@mozilla.org/PopupWindowManager;1"]
>-                     .getService(Components.interfaces.nsIPopupWindowManager);
>-          blocking = PM.testPermission(this.popupURL) ==
>-                     Components.interfaces.nsIPopupWindowManager.DENY_POPUP;
>+          blocking = Services.perms.testPermission(this.popupURL, "popup") ==
>+                     Services.perms.DENY_ACTION;
Unfortunately the popup window manager's version does more than this.

One option is to rename initPopupURL to initPopupPrincipal and fix the code ;-) [opener.document.nodePrincipal seems to be the way to get one]

Comment 4

[Philip Chee]

Attachment: Patch v1.1 Use opener.document.nodePrincipal

>># User Neil Rashbrook <[e-mail]>
> Eek! Imposter Alert!
I blame TortoiseHg's auto-suggest.

>>-          const PM = Components.classes["@mozilla.org/PopupWindowManager;1"]
>>-                     .getService(Components.interfaces.nsIPopupWindowManager);
>>-          blocking = PM.testPermission(this.popupURL) ==
>>-                     Components.interfaces.nsIPopupWindowManager.DENY_POPUP;
>>+          blocking = Services.perms.testPermission(this.popupURL, "popup") ==
>>+                     Services.perms.DENY_ACTION;
> Unfortunately the popup window manager's version does more than this.
> 
> One option is to rename initPopupURL to initPopupPrincipal and fix the code
> ;-) [opener.document.nodePrincipal seems to be the way to get one]
Fixed.

Comment 5

[[e-mail]]

Comment on attachment 646807 [details] [diff] [review]
Patch v1.1 Use opener.document.nodePrincipal

>-          const PM = Components.classes["@mozilla.org/PopupWindowManager;1"]
>-                     .getService(Components.interfaces.nsIPopupWindowManager);
>+          const PM = this.getService("@mozilla.org/PopupWindowManager;1",
>+                                     "nsIPopupWindowManager");
Nobody else uses that method, no reason to change it. r=me with that fixed.

Comment 6

[Philip Chee]

>>-          const PM = Components.classes["@mozilla.org/PopupWindowManager;1"]
>>-                     .getService(Components.interfaces.nsIPopupWindowManager);
>>+          const PM = this.getService("@mozilla.org/PopupWindowManager;1",
>>+                                     "nsIPopupWindowManager");
> Nobody else uses that method, no reason to change it. r=me with that fixed.
Fixed.

I also changed:
Services.console.logStringMessage(e)
to:
Components.utils.reportError(e);

Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/fbd7b407ecb1