Wrong pop-up message is displayed when running while(true) {window.open(...);}

VERIFIED FIXED in Firefox 66

Status

()

defect
P1
normal
VERIFIED FIXED
5 months ago
3 months ago

People

(Reporter: mboldan, Assigned: Paolo)

Tracking

({regression})

Trunk
mozilla67
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox64 unaffected, firefox65 wontfix, firefox66 verified, firefox67 verified)

Details

Attachments

(1 attachment)

Reporter

Description

5 months ago

Affected versions

  • Firefox 65.0b9
  • Firefox 66.0a1 (2019-01-09)

Affected platforms

  • Windows 10x64
  • Ubuntu 18.04x64
  • macOS 10.12.6

Steps to reproduce

  1. Launch Firefox.
  2. Open Web Console.
  3. Run this script while (true) {window.open('http://google.fi');} and observe the displayed pop-up.

Expected result

  • Nightly prevented this site from opening more than 100 pop-up windows. message is displayed.

Actual result

  • Nightly prevented this site from opening a pop-up window message is displayed.

Regression range

Additional notes

  • On Firefox 64.0.2 RC it's displayed exactly the no. of pop-ups that are prevented for opening. It seems that this is the old behavior.

Bug 1496827 looks like a possible candidate there?

Blocks: 1496827
Flags: needinfo?(paolo.mozmail)

Brian, flagging you for needinfo since you reviewed the offending patch. Could you take a look since Paolo hasn't gotten to it yet?

Flags: needinfo?(bgrinstead)

Too late for 65 at this point.

Assignee

Comment 5

4 months ago

This fixes the notification bar that shows how many popup windows have been blocked.

Assignee

Comment 6

4 months ago

This was lower priority, but it turns out it is also an easy fix.

Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Component: Security → XUL Widgets
Flags: needinfo?(paolo.mozmail)
Flags: needinfo?(bgrinstead)
Priority: -- → P1
Product: Firefox → Toolkit
Has Regression Range: --- → yes

Comment 7

4 months ago
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/00074091644d
Allow changing the text of an existing notification. r=jaws

Backed out changeset 00074091644d (bug 1519095) for browser-chrome failures at browser/base/content/test/popups/browser_popup_blocker.js

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/cefe669c7351fc07d33f1c00979c81fdd7933d20

Failure push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&selectedJob=223521216&revision=00074091644d1aab8e83060c14f63e3104089f68

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=223521742&repo=mozilla-inbound&lineNumber=2757

08:12:21 INFO - TEST-START | browser/base/content/test/popups/browser_popupUI.js
08:12:22 INFO - GECKO(925) | Can't find symbol 'GetGraphicsResetStatus'.
08:12:22 INFO - GECKO(925) | MEMORY STAT vsizeMaxContiguous not supported in this build configuration.
08:12:22 INFO - GECKO(925) | MEMORY STAT | vsize 4157MB | residentFast 299MB | heapAllocated 129MB
08:12:22 INFO - TEST-OK | browser/base/content/test/popups/browser_popupUI.js | took 1152ms
08:12:23 INFO - checking window state
08:12:23 INFO - TEST-START | browser/base/content/test/popups/browser_popup_blocker.js
08:12:23 INFO - TEST-INFO | started process screencapture
08:12:24 INFO - TEST-INFO | screencapture: exit 0
08:12:24 INFO - Buffered messages logged at 08:12:23
08:12:24 INFO - Entering test bound setup
08:12:24 INFO - Leaving test bound setup
08:12:24 INFO - Entering test bound test_maximum_reported_blocks
08:12:24 INFO - Buffered messages finished
08:12:24 INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/popups/browser_popup_blocker.js | Uncaught exception - at chrome://mochitests/content/browser/browser/base/content/test/popups/browser_popup_blocker.js:35 - TypeError: notification.label is undefined
08:12:24 INFO - Stack trace:
08:12:24 INFO - test_maximum_reported_blocks@chrome://mochitests/content/browser/browser/base/content/test/popups/browser_popup_blocker.js:35:3
08:12:24 INFO - AsyncTester_execTest/<@chrome://mochikit/content/browser-test.js:1108:34
08:12:24 INFO - async
Tester_execTest@chrome://mochikit/content/browser-test.js:1099:16
08:12:24 INFO - nextTest/<@chrome://mochikit/content/browser-test.js:997:9
08:12:24 INFO - SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:803:59
08:12:24 INFO - Leaving test bound test_maximum_reported_blocks
08:12:24 INFO - Entering test bound test_opening_blocked_popups
08:12:24 INFO - TEST-PASS | browser/base/content/test/popups/browser_popup_blocker.js | Blocked popup menu shown -
08:12:24 INFO - TEST-PASS | browser/base/content/test/popups/browser_popup_blocker.js | Two popups were blocked -
08:12:24 INFO - Console message: [JavaScript Error: "The character encoding of the HTML document was not declared. The document will render with garbled text in some browser configurations if the document contains characters from outside the US-ASCII range. The character encoding of the page must be declared in the document or in the transfer protocol." {file: "http://example.com/browser/browser/base/content/test/popups/popup_blocker_b.html" line: 0}]
08:12:24 INFO - Console message: [JavaScript Error: "The character encoding of the HTML document was not declared. The document will render with garbled text in some browser configurations if the document contains characters from outside the US-ASCII range. The character encoding of the page must be declared in the document or in the transfer protocol." {file: "http://example.com/browser/browser/base/content/test/popups/popup_blocker_a.html" line: 0}]
08:12:24 INFO - TEST-PASS | browser/base/content/test/popups/browser_popup_blocker.js | Popup a -
08:12:24 INFO - TEST-PASS | browser/base/content/test/popups/browser_popup_blocker.js | Popup b -
08:12:24 INFO - Leaving test bound test_opening_blocked_popups
08:12:24 INFO - GECKO(925) | MEMORY STAT | vsize 4206MB | residentFast 333MB | heapAllocated 136MB
08:12:24 INFO - TEST-OK | browser/base/content/test/popups/browser_popup_blocker.js | took 1732ms
08:12:24 INFO - Not taking screenshot here: see the one that was previously logged
08:12:24 INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/popups/browser_popup_blocker.js | Found an unexpected tab at the end of test run: http://example.com/browser/browser/base/content/test/popups/popup_blocker_10_popups.html -
08:12:24 INFO - checking window state
08:12:24 INFO - TEST-START | browser/base/content/test/popups/browser_popup_blocker_identity_block.js
08:12:25 INFO - GECKO(925) | ###!!! [Parent][DispatchAsyncMessage] Error: PQuotaUsageRequest::Msg_Cancel Route error: message sent to unknown actor ID
08:12:28 INFO - GECKO(925) | MEMORY STAT | vsize 4449MB | residentFast 344MB | heapAllocated 119MB

Flags: needinfo?(paolo.mozmail)

Comment 9

4 months ago
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8aeed3352c0
Allow changing the text of an existing notification. r=jaws
Assignee

Updated

4 months ago
Flags: needinfo?(pamadini)

Comment 10

4 months ago
bugherder
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
Assignee

Comment 11

4 months ago

Comment on attachment 9038040 [details]
Bug 1519095 - Allow changing the text of an existing notification. r=jaws

Beta/Release Uplift Approval Request

Feature/Bug causing the regression

Bug 1496827

User impact if declined

The notification bar for blocked pop-ups won't mention how many were blocked

Is this code covered by automated tests?

Yes

Has the fix been verified in Nightly?

No

Needs manual test from QE?

Yes

If yes, steps to reproduce

Per comment 0

List of other uplifts needed

None

Risk to taking this patch

Low

Why is the change risky/not risky? (and alternatives if risky)

Simple code change, the issue wasn't detected by tests but they've now been updated to check the actual label

String changes made/needed

None

Attachment #9038040 - Flags: approval-mozilla-beta?
Whiteboard: [qa-triaged]
Reporter

Comment 12

3 months ago

I managed to reproduce this issue on Firefox 66.0b7, under Windows 10x64.
The issue is no longer reproducible on Firefox 67.0a1 (2019-02-13).
Tests were executed under Window 10x64, macOS 10.12.6 and under Ubuntu 16.04x64.
I will mark the issue Verified Fixed and remove the 'qe-verify +' flag as soon as it gets verified on a beta build.

Comment on attachment 9038040 [details]
Bug 1519095 - Allow changing the text of an existing notification. r=jaws

Let's uplift this for beta 8 and get further verification there as Mihai suggests.

Attachment #9038040 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Reporter

Comment 15

3 months ago

(In reply to Mihai Boldan, QA [:mboldan] from comment #12)

I managed to reproduce this issue on Firefox 66.0b7, under Windows 10x64.
The issue is no longer reproducible on Firefox 67.0a1 (2019-02-13).
Tests were executed under Window 10x64, macOS 10.12.6 and under Ubuntu
16.04x64.
I will mark the issue Verified Fixed and remove the 'qe-verify +' flag as
soon as it gets verified on a beta build.

I've also verified the issue on Firefox 66.0b8, under Windows 10x64, Mac OS X 10.11.6 and under Ubuntu 18.04x64.
Marking this issue as Verified Fixed.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
QA Whiteboard: [qa-triaged]
Whiteboard: [qa-triaged]
You need to log in before you can comment on or make changes to this bug.