1273685 - PopupBlocking:UpdateBlockedPopups messages can be very large

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect, P1)

Description

[Andrew McCreight [:mccr8]]

Looking at telemetry, the PopupBlocking:UpdateBlockedPopups message can get pretty large. There are maybe a million messages 800kb or larger on beta e10s right now, compared to around 80 million SessionStore:update messages. So, not critical, but this would be nice to fix if possible.

It looks like maybe popupData just gets bigger and bigger, and the entirety of it gets sent to the parent process every time, so maybe if you sit there on a page that is trying to spam popups the message gets large? I guess also if a site is opening a popup with a data URI it could be extremely long.

Comment 1

[:Felipe Gomes (needinfo for replies!)]

Attachment: Do not send URL list with the UpdateBlockedPopups message

So the popup blocking code does some incredibly unoptimized things, even looking back to its pre-e10s era. It sends the data on every pageshow message which can be triggered by iframes and it's not clear that it correctly filters them.

Anyway, the biggest issue is that it sends the URLs of all the popups blocked in a document on each pageshow. However, Firefox only displays the count of popups in the main UI, and the URL list is only used when the user clicks on the button to open the menu (allowing them to pick a popup to unblock).

So what I'm changing is that the content process will only send the count of popups blocked, and the parent will only request the URL list when it needs it to be displayed. I'm also clamping the list to 15 URLs because anything as large as that is already unusable for an end user to choose.

Looking at the code, there's a lot more that can be improved, but this change should alleviate the important issue here, and we can leave other improvements out of scope here. It will not reduce the amount of messages but will make them very small and fixed in size.

browser_popup_blocker.js is passing with no changes but I still need to fix browser_privatebrowsing_popupblocker.js and look to see if there are other tests that need changes. Posting the almost ready patch here for posterity.

Comment 2

[:Gijs (he/him)]

(just braindumping...)

When I looked at this a while back, I remember wondering why we didn't just send the 'new' blocked popups information as it became available, rather than all of it. Is that an option at all?

I also wondered if for things like data: URIs, each URI having some kind of unique ID and only sending the first ~255 characters might help further reduce the amount of traffic. No idea how hard that is to actually implement, though...

Comment 3

[:Felipe Gomes (needinfo for replies!)]

(In reply to :Gijs Kruitbosch from comment #2)
> (just braindumping...)
> 
> When I looked at this a while back, I remember wondering why we didn't just
> send the 'new' blocked popups information as it became available, rather
> than all of it. Is that an option at all?

Not sure I understood what you mean. I imagine you're talking about the cached browser.blockedPopups list on the parent side, which is refreshed every time with all the info instead of just the newly blocked popup.  If yes, then this patch does it because that list is not kept on the parent side anymore, as we will just send an integer with the count, instead of the full list.

> 
> I also wondered if for things like data: URIs, each URI having some kind of
> unique ID and only sending the first ~255 characters might help further
> reduce the amount of traffic. No idea how hard that is to actually
> implement, though...

Yeah that's a good idea, and would be super easy. The URL is only used be displayed in the menu popup, and long ones will get cropped anyways. The API to actually unblock the popups uses an index on the list and not the full URL.

Comment 4

[:Felipe Gomes (needinfo for replies!)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f83b045bd064

Comment 5

[:Felipe Gomes (needinfo for replies!)]

Attachment: MozReview Request: Bug 1273685. Reduce size of PopupBlocking:UpdateBlockedPopups messages. r=Gijs

The size is reduced by making the message only carry the count of blocked popups, and not the full list of popup URLs that were blocked. The parent becomes responsible for retrieving the list from the child when it needs to display it.

Review commit: https://reviewboard.mozilla.org/r/55690/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55690/

Comment 6

[:Felipe Gomes (needinfo for replies!)]

Attachment: MozReview Request: Bug 1273685. Further reduce the message size by limiting the size of the URI to be sent. r=Gijs

Review commit: https://reviewboard.mozilla.org/r/55692/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55692/

Comment 7

[:Gijs (he/him)]

Comment on attachment 8757143 [details]
MozReview Request: Bug 1273685. Reduce size of PopupBlocking:UpdateBlockedPopups messages. r=Gijs

https://reviewboard.mozilla.org/r/55690/#review52426

So, no r+ because I think we should avoid trying to construct URIs out of the URLs we're truncating in the second patch for reasons outlined below. Though if you prefer, I guess, take it as r+ and leave the rest/truncating for a followup bug?

The other thing that does also kind of worry me is that this will mean the popup menu is initially empty, and we have to roundtrip to the child process to get the information. If we send the URLs in question one at a time (as they're blocked), that'll allow the parent to keep the list independently and without a further roundtrip to the child... of course, at the cost of sending the URLs even if the popup isn't opened. I guess it depends what tradeoffs we want... it's not a great experience if you click the menu and the site generating all these popups is keeping the child process busy...

Should we keep updating the count of popups after we pass 15 ? Or just make the UI display "More than 15" or something? Feels like if a site spams you with popups this will still generate traffic (just a lot of small messages instead of big ones)... Not sure if that's worth doing here though.

::: browser/base/content/browser.js:579
(Diff revision 1)
> -    var blockedPopups = browser.blockedPopups;
> +    // event.target.anchorNode is cleared when the event dispatch finishes,
> +    // so we store it as a closure here to use inside the promise.then() function.
> +    let anchorNode = aEvent.target.anchorNode;

Do we not need to do the same with aEvent.target, which is also used?

::: browser/base/content/browser.js:600
(Diff revision 1)
> -        // isn't useful (for instance, netscape.com's popup URI ends up
> +          // isn't useful (for instance, netscape.com's popup URI ends up
> -        // being "http://www.netscape.com", which isn't really the URI of
> +          // being "http://www.netscape.com", which isn't really the URI of
> -        // the popup they're trying to show).  This isn't going to be
> +          // the popup they're trying to show).  This isn't going to be
> -        // useful to the user, so we won't create a menu item for it.
> +          // useful to the user, so we won't create a menu item for it.
> -        if (popupURIspec == "" || popupURIspec == "about:blank" ||
> +          if (popupURIspec == "" || popupURIspec == "about:blank" ||
> -            popupURIspec == uri.spec)
> +              popupURIspec == uri.spec)

This check will also break when we start truncating URIs. We should probably send some kind of special "<self>" identifier from the client in that case.

::: toolkit/content/browser-content.js:365
(Diff revision 1)
> +        count: this.popupData ? this.popupData.length : 0,
> +        freshPopup: freshPopup

Nit: If we're changing this anyway, might as well use ES<whatever> shorthand and just use `freshPopup,`

(please also mind the trailing comma either way.)

::: toolkit/content/widgets/browser.xml:670
(Diff revision 1)
>  
> +      <method name="retrieveListOfBlockedPopups">
> +        <body>
> +          <![CDATA[
> +          this.messageManager.sendAsyncMessage("PopupBlocking:GetBlockedPopupList", null);
> +          return new Promise((resolve) => {

Nit: Don't need () around a single argument

::: toolkit/content/widgets/browser.xml:679
(Diff revision 1)
> +                    let scope = Components.utils.import("resource://gre/modules/BrowserUtils.jsm", {});
> +                    let uri = scope.BrowserUtils.makeURI(list[i].popupWindowURI);
> +                    list[i].popupWindowURI = uri;

This looks OK here, but note that the limiting in the other patch makes this fragile. If the spec includes percent-encoded things, and we substring in the middle of those, that will break them and I would expect makeURI() to balk at them.

Is it really necessary to have URL objects here?

Comment 8

[:Gijs (he/him)]

Comment on attachment 8757144 [details]
MozReview Request: Bug 1273685. Further reduce the message size by limiting the size of the URI to be sent. r=Gijs

https://reviewboard.mozilla.org/r/55692/#review52428

::: toolkit/content/browser-content.js:298
(Diff revision 1)
> +
> +        for (let i = 0; i < length; i++) {
> +          popupData.push({
> +            // Limit 1000 chars to be sent because the URI will be cropped
> +            // by the UI anyway, and data: URIs can be significantly larger.
> +            popupWindowURIspec: this.popupData[i].popupWindowURIspec.substring(0, 1000)

As noted earlier, this is going to be breaking the equality check with the browser's currentURI, and it might break the URI constructor if you truncate in the middle of escape sequences.

Also... should we ellipsize the URLs we truncate?

Comment 9

[:Felipe Gomes (needinfo for replies!)]

https://reviewboard.mozilla.org/r/55690/#review52472

::: browser/base/content/browser.js:579
(Diff revision 1)
> -    var blockedPopups = browser.blockedPopups;
> +    // event.target.anchorNode is cleared when the event dispatch finishes,
> +    // so we store it as a closure here to use inside the promise.then() function.
> +    let anchorNode = aEvent.target.anchorNode;

Apparently not, only the anchorNode is cleared

::: browser/base/content/browser.js:600
(Diff revision 1)
> -        // isn't useful (for instance, netscape.com's popup URI ends up
> +          // isn't useful (for instance, netscape.com's popup URI ends up
> -        // being "http://www.netscape.com", which isn't really the URI of
> +          // being "http://www.netscape.com", which isn't really the URI of
> -        // the popup they're trying to show).  This isn't going to be
> +          // the popup they're trying to show).  This isn't going to be
> -        // useful to the user, so we won't create a menu item for it.
> +          // useful to the user, so we won't create a menu item for it.
> -        if (popupURIspec == "" || popupURIspec == "about:blank" ||
> +          if (popupURIspec == "" || popupURIspec == "about:blank" ||
> -            popupURIspec == uri.spec)
> +              popupURIspec == uri.spec)

Good point.  I split all of the URI truncating to the second patch to make it optional (as the 1st patch is the important one to fix this bug). But I'll try to come up with something for the second patch to avoid this issue.

(marking it as Fixed here because i'm leaving it for the other patch)

::: toolkit/content/browser-content.js:365
(Diff revision 1)
> +        count: this.popupData ? this.popupData.length : 0,
> +        freshPopup: freshPopup

will do

::: toolkit/content/widgets/browser.xml:679
(Diff revision 1)
> +                    let scope = Components.utils.import("resource://gre/modules/BrowserUtils.jsm", {});
> +                    let uri = scope.BrowserUtils.makeURI(list[i].popupWindowURI);
> +                    list[i].popupWindowURI = uri;

The other patch already removes this URL-construction code and only uses strings :)

Comment 10

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8757143 [details]
MozReview Request: Bug 1273685. Reduce size of PopupBlocking:UpdateBlockedPopups messages. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55690/diff/1-2/

Comment 11

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8757144 [details]
MozReview Request: Bug 1273685. Further reduce the message size by limiting the size of the URI to be sent. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55692/diff/1-2/

Comment 12

[:Felipe Gomes (needinfo for replies!)]

https://reviewboard.mozilla.org/r/55690/#review52928

::: browser/base/content/browser.js:580
(Diff revision 2)
> +    blockedPopupDontShowMessage.setAttribute("checked", !showMessage);
> +    if (aEvent.target.anchorNode.id == "page-report-button") {
> +      aEvent.target.anchorNode.setAttribute("open", "true");
> +      blockedPopupDontShowMessage.setAttribute("label", gNavigatorBundle.getString("popupWarningDontShowFromLocationbar"));
> +    } else
> +      blockedPopupDontShowMessage.setAttribute("label", gNavigatorBundle.getString("popupWarningDontShowFromMessage"));
> +
> +    let blockedPopupsSeparator =
> +        document.getElementById("blockedPopupsSeparator");
> +    blockedPopupsSeparator.setAttribute("hidden", true);

By bringing this outside of the promise callback, the menu will show up nicely while waiting for the list of popups to arrive http://i.imgur.com/4UCA0nU.png

Also that trick of storing the anchorNode is no longer needed

Comment 13

[:Felipe Gomes (needinfo for replies!)]

https://reviewboard.mozilla.org/r/55692/#review52428

> As noted earlier, this is going to be breaking the equality check with the browser's currentURI, and it might break the URI constructor if you truncate in the middle of escape sequences.
> 
> Also... should we ellipsize the URLs we truncate?

Fixed in the new patch by introducing a `<self>` annotation.

No need to ellipsize because the UI already does it.

I reduced the limit to 500 because that's already too much.

Comment 14

[:Felipe Gomes (needinfo for replies!)]

https://reviewboard.mozilla.org/r/55692/#review52934

::: toolkit/content/browser-content.js:298
(Diff revision 2)
> +
> +        for (let i = 0; i < length; i++) {
> +
> +          let popupWindowURIspec = this.popupData[i].popupWindowURIspec;
> +
> +          if (popupWindowURIspec = global.content.location.href) {

argh, s/=/==/

Comment 15

[:Gijs (he/him)]

Comment on attachment 8757143 [details]
MozReview Request: Bug 1273685. Reduce size of PopupBlocking:UpdateBlockedPopups messages. r=Gijs

https://reviewboard.mozilla.org/r/55690/#review52942

::: browser/base/content/browser.js:584
(Diff revision 2)
> +    } else
> +      blockedPopupDontShowMessage.setAttribute("label", gNavigatorBundle.getString("popupWarningDontShowFromMessage"));
> +

Nit: if braces around the if block, then also braces around the else block.

Comment 16

[:Gijs (he/him)]

Comment on attachment 8757144 [details]
MozReview Request: Bug 1273685. Further reduce the message size by limiting the size of the URI to be sent. r=Gijs

https://reviewboard.mozilla.org/r/55692/#review52944

::: browser/base/content/browser.js:610
(Diff revision 2)
> +              popupURIspec == "<self>" ||
>                popupURIspec == uri.spec)

Do we still need the second check?

::: toolkit/content/browser-content.js:295
(Diff revision 2)
> -                         { popupData: this.popupData ?
> -                                      this.popupData.slice(0, 15) :
> -                                      [] });
> +
> +        // Limit 15 popup URLs to be reported through the UI
> +        length = Math.min(length, 15);
> +
> +        for (let i = 0; i < length; i++) {
> +

Nit: useless empty line.

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/6421e7c224d7
https://hg.mozilla.org/integration/fx-team/rev/3a87296fe414

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6421e7c224d7
https://hg.mozilla.org/mozilla-central/rev/3a87296fe414

Comment 19

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8757143 [details]
MozReview Request: Bug 1273685. Reduce size of PopupBlocking:UpdateBlockedPopups messages. r=Gijs

Approval Request Comment
[Feature/regressing bug #]: e10s - The popup blocking feature sends too many + too large messages between processes, potentially contributing to OOM issues on e10s.
[User impact if declined]: larger messages going through the IPC channel which requires more memory and slow down other messages
[Describe test coverage new/current, TreeHerder]: landed on central. the popup blocker feature is covered by tests which are passing
[Risks and why]: the patch is definitely not risk free, but I believe it is well contained. There are no changes to the actual popup blocking, only to the UI that presents it and allows popups to be unblocked.
[String/UUID change made/needed]: none

Comment 20

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8757144 [details]
MozReview Request: Bug 1273685. Further reduce the message size by limiting the size of the URI to be sent. r=Gijs

Approval Request Comment
[Feature/regressing bug #]: this is a companion patch to the other one, which helps reduce the message size in case a long "data:text/html,..." url was blocked, by trimming it a maximum size.

The first patch one is more important to fix the bug, but I don't see this one adding too much risk beyond the first one.
[User impact if declined]: not much besides for data: urls blocked popups, and code differing from central
[Describe test coverage new/current, TreeHerder]: landed on central, feature covered by tests
[Risks and why]: same as above
[String/UUID change made/needed]: none

Comment 21

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8757143 [details]
MozReview Request: Bug 1273685. Reduce size of PopupBlocking:UpdateBlockedPopups messages. r=Gijs

Approval Request Comment:
Changing request to beta

Comment 22

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8757144 [details]
MozReview Request: Bug 1273685. Further reduce the message size by limiting the size of the URI to be sent. r=Gijs

Approval Request Comment
Changing request to beta

Comment 23

[Sylvestre Ledru [:Sylvestre]]

(In reply to :Felipe Gomes (needinfo me!) from comment #19)
> [Feature/regressing bug #]: e10s - The popup blocking feature sends too many
> + too large messages between processes, potentially contributing to OOM
> issues on e10s.
As we are planning to ship e10s soon, I am concerned about potential regressions. 
Do you have an idea if the decrease of OOM is going to be significant or not?
(I know it is hard to tell, I don't need proof)

Comment 24

[:Felipe Gomes (needinfo for replies!)]

It is hard to tell, but from some conversations I had, it looks this would be a good improvement, but is not essential.. I think we're fine waiting for it on 49.

Comment 25

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8757143 [details]
MozReview Request: Bug 1273685. Reduce size of PopupBlocking:UpdateBlockedPopups messages. r=Gijs

OK, let it ride the train then!

Comment 26

[Chris Peterson [:cpeterson]]

wontfix for Beta 48 based on Felipe's comment 24.