If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Infinite recursion in browserAction popup resizing logic with media queries

VERIFIED FIXED in Firefox 50

Status

()

Toolkit
WebExtensions: Untriaged
P1
normal
VERIFIED FIXED
a year ago
10 months ago

People

(Reporter: robwu, Assigned: kmag)

Tracking

(Blocks: 1 bug)

50 Branch
mozilla50
Points:
---

Firefox Tracking Flags

(firefox50 verified, firefox51 verified)

Details

(Whiteboard: triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

a year ago
Created attachment 8778672 [details]
browserAction-popup-size.zip

1. Load the attached addon.
2. Click on the extension button to open the popup window.

Expected:
- The popup opens and the displayed page should be red (=media query max-width:700px)

Actual:
- The popup flashes color and resizes.
- The browser is not responsive.
- Eventually, the Unresponsive script dialog appears, at http://searchfox.org/mozilla-central/rev/389a3e0817b873a64376ec2b65f6807afbba9d8f/browser/base/content/tabbrowser.xml#5405

I initially experienced this when I loaded example.com in the browserAction popup. When I looked at the console I saw an Infinite recursion error at http://searchfox.org/mozilla-central/rev/389a3e0817b873a64376ec2b65f6807afbba9d8f/browser/components/extensions/ext-utils.js#277-278. But I did not see this again when I tried to create a reduction (minimal test case).

Tested with Firefox 51.0a1 2016-08-04 on OS X.
With Mozregression I narrowed the commit range to [437fc937, 0e3f8401] and guess that the following are suspect:
https://hg.mozilla.org/mozilla-central/rev/6e96476b8d49 <-- not this bug, but appearance of popup changed (previously the popup was yellow and did not resize again) and setTimeout error appears in the console.
https://hg.mozilla.org/mozilla-central/rev/d490ad95a673 <-- Fixed setTimeout error, now the popup flashes all the time.
Created attachment 8778698 [details]
Bug 1293036: Prevent BasePopup._resizeBrowser from being called recursive from MozScrolledAreaChanged events.

This doesn't include tests for this behavior, because I'm not sure there's a
good way to test it reliably.

Review commit: https://reviewboard.mozilla.org/r/69866/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/69866/
Attachment #8778698 - Flags: review?(rob)
This is probably going to need uplift to 50.
(Reporter)

Comment 3

a year ago
Comment on attachment 8778698 [details]
Bug 1293036: Prevent BasePopup._resizeBrowser from being called recursive from MozScrolledAreaChanged events.

https://reviewboard.mozilla.org/r/69866/#review66972

I manually verified with the test case from the bug report that this patch fixes the issue.`

But please do add a test to avoid future regressions. Although the reported bug is hard to test exactly, it can be approximated: the issue is that the panel was resized very often in a short timespan. So I suggest to measure the number of resizes (e.g. via resize events or media query events - https://developer.mozilla.org/en-US/docs/Web/CSS/Media_Queries/Testing_media_queries), and if a threshold in some timespan is exceeded, fail the test. Like this:

    // Assuming that the popup is already open when this test logic starts.

    const RESIZE_TIMEOUT = 100; // keep in sync with ext-utils.js
    const CYCLES_FOR_TESTING = 5;
    let i = CYCLES_FOR_TESTING;
    setTimeout(function again() {
      if (--i >= 0) {
        setTimeout(next, RESIZE_TIMEOUT);
      } else {
        // We expect only one resize per cycle, but let's choose a conservative
        // threshold (2x) to avoid intermittents.
        assert(todo_some_resize_counter <= 2 * CYCLES_FOR_TESTING);
      }
    }, RESIZE_TIMEOUT);

It is a bit fuzzy, but at least it would catch severe bugs like the one I reported.

::: browser/components/extensions/ext-utils.js:219
(Diff revision 1)
>    }
>    // Resizes the browser to match the preferred size of the content (debounced).
>    resizeBrowser() {
>      if (this.resizeTimeout == null) {
> -      this._resizeBrowser();
>        this.resizeTimeout = this.window.setTimeout(this._resizeBrowser.bind(this), RESIZE_TIMEOUT);

I think that it's clearer to keep the knowledge of the timer in the `resizeBrowse` method only, and use:


    this.resizeTimeout = this.window.setTimeout(() => {
      this.resizeTimeout = null;
      this._resizeBrowser();
    }, RESIZE_TIMEOUT);
    this._resizeBrowser();
Attachment #8778698 - Flags: review?(rob) → review+

Updated

a year ago
Whiteboard: triaged
https://hg.mozilla.org/integration/mozilla-inbound/rev/65e98f10c1c753359312ea9d78ac602406065a0b
Bug 1293036: Prevent BasePopup._resizeBrowser from being called recursive from MozScrolledAreaChanged events. r=robwu
Comment on attachment 8778698 [details]
Bug 1293036: Prevent BasePopup._resizeBrowser from being called recursive from MozScrolledAreaChanged events.

Approval Request Comment
[Feature/regressing bug #]: Bug 1215025
[User impact if declined]: This issue has the potential to cause infinite recursion errors that cause the browser to freeze, when extensions load certain types of web pages into panels. It is probably more likely to impact developers than end users, at this point.
[Describe test coverage new/current, TreeHerder]: The related functionality is well-covered by regression tests, but there is currently no test coverage to reproduce this bug.
[Risks and why]: Low. The changes are minimal, and essentially amount to checking a flag to prevent reentrancy.
[String/UUID change made/needed]: None.
Attachment #8778698 - Flags: approval-mozilla-aurora?

Comment 6

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/65e98f10c1c7
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox51: --- → fixed
Resolution: --- → FIXED

Updated

a year ago
status-firefox50: --- → affected
Comment on attachment 8778698 [details]
Bug 1293036: Prevent BasePopup._resizeBrowser from being called recursive from MozScrolledAreaChanged events.

Has stabilized on Nightly, prevents browser freeze in some scenarios, Aurora50+
Attachment #8778698 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 8

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/0ee3a401d390
status-firefox50: affected → fixed
Blocks: 1237377

Comment 9

a year ago
Related to this issue, I observed that "- The popup flashes color and resizes." is still reproducing.

This issue was tested on Firefox 51.0a1 (2016-09-02) and Firefox 50.0a2 (2016-09-02). 
Any thoughts about this?
Flags: needinfo?(kmaglione+bmo)
(In reply to CosminB from comment #9)
> Related to this issue, I observed that "- The popup flashes color and
> resizes." is still reproducing.

That's expected, so long as it doesn't go into an infinite loop.
Flags: needinfo?(kmaglione+bmo)

Comment 11

a year ago
The popup flashes color and resizes but does not go into an infinite loop so based on :kmag answer this issue is verified as fixed on Firefox 51.0a1 (2016-09-04) and Firefox 50.0a2 (2016-09-04) under Windows 7 64-bit.

Updated

a year ago
status-firefox50: fixed → verified
status-firefox51: fixed → verified

Updated

a year ago
Status: RESOLVED → VERIFIED
Assignee: nobody → kmaglione+bmo
You need to log in before you can comment on or make changes to this bug.