Closed Bug 778076 Opened 9 years ago Closed 9 years ago

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

Categories

(SeaMonkey :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.14

People

(Reporter: philip.chee, Assigned: philip.chee)

References

Details

Attachments

(1 file, 2 obsolete files)

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
Attached file Patch v1.0 Proposed Fix. (obsolete) —
Attached file Patch v1.0 Proposed Fix. (obsolete) —
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
Assignee: nobody → philip.chee
Attachment #646509 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #646510 - Flags: review?(neil)
Comment on attachment 646510 [details]
Patch v1.0 Proposed Fix.

># User Neil Rashbrook <neil@parkwaycc.co.uk>
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]
Attachment #646510 - Flags: review?(neil) → review-
Blocks: 769586
>># User Neil Rashbrook <neil@parkwaycc.co.uk>
> 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.
Attachment #646510 - Attachment is obsolete: true
Attachment #646807 - Flags: review?(neil)
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.
Attachment #646807 - Flags: review?(neil) → review+
>>-          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
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.14
You need to log in before you can comment on or make changes to this bug.