Omitted maxResults property not handled correctly in getRecentlyClosed

👁 Image	YF (Yang) Reporter
	Description • 8 years ago

Issue: See https://hg.mozilla.org/mozilla-central/annotate/5ca5691372cb432ec1fa4693ca608a30858226de/browser/components/extensions/ext-sessions.js#l65, the "let maxResults" got undefined as "filter.maxResults" is null while not given maxResults. Actual results: browser.sessions.getRecentlyClosed() return 32 items instead of 25 items (MAX_SESSION_RESULTS). Expected results: let maxResults = filter.maxResults === null ? this.MAX_SESSION_RESULTS : filter.maxResults; Find out why this issue appears. new tests.

👁 Image	YF (Yang) Reporter
	Comment 1 • 8 years ago

STR: 1. Opening and closing more than 30 tabs. 2. Temporarily load the test.xpi from about:debugging#addons. 3. Check the Browser Console, or use Browser Toolbox explore the ext-sessions.js, set breakpoint on line and reload the xpi.

👁 Image	Kris Maglione [:kmag]
	Comment 2 • 8 years ago

I don't see a particular reason to enforce this limit. The issue is the number of closed tabs we store, not the number we return. And we enforce our limit per-window (by default 10) rather than globally.

👁 Image	YF (Yang) Reporter
	Comment 3 • 8 years ago

I don't fully understand what you mean. The line (maxResults) will take effect when ...? I see the browser.sessions.getRecentlyClosed() returned items exceeds the number of the value.

👁 Image	YF (Yang) Reporter
	Comment 4 • 8 years ago

> And we enforce our limit per-window (by default 10) rather than globally. I see it now. Maybe the Tab Groups 2.1.4 isolate and affect this number for me.

👁 Image	Kris Maglione [:kmag]
	Comment 5 • 8 years ago

(In reply to YF (Yang) from comment #3) > I don't fully understand what you mean. The line (maxResults) will take > effect when ...? > > I see the browser.sessions.getRecentlyClosed() returned items exceeds the > number of the value. Ah, I see what you mean now. I thought you meant that we don't restrict the `maxResults` parameter to `MAX_SESSION_RESULTS`... which we unfortunately do, but shouldn't. But the actual problem is that we don't handle an omitted maxResults value correctly.

👁 Image	Kris Maglione [:kmag]
	Updated • 8 years ago

👁 Image	Caitlin Neiman [:caitmuenster]
	Updated • 8 years ago

👁 Image	Andy McKay
	Updated • 8 years ago

👁 Image	BMO Automation
	Updated • 7 years ago

👁 Image	Sandeep Bypina [:sandeepb]
	Updated • 7 years ago

👁 Image	Vinoth
	Comment 6 • 7 years ago

Is this bug assigned to anyone, could I take it if not? New contributor here and this was under 'Good First Bugs'. Could you direct me to the file in which it is present? And the specific lines of code?

👁 Image	YF (Yang) Reporter
	Comment 7 • 7 years ago

(In reply to Vinoth from comment #6) > Is this bug assigned to anyone, could I take it if not? New contributor here > and this was under 'Good First Bugs'. Could you direct me to the file in > which it is present? And the specific lines of code? https://dxr.mozilla.org/mozilla-central/search?q=maxResults+%3D+filter.maxResults

👁 Image	Caitlin Neiman [:caitmuenster]
	Updated • 7 years ago

👁 Image	Caitlin Neiman [:caitmuenster]
	Comment 8 • 7 years ago

Welcome, Vinoth, and thanks for the link to the code, YF! Vinoth, if you need any help, please check the box that says "need more information from" and select "mentor" to ping :zombie. :)

👁 Image	Arshad Kazmi [:arshadkazmi42]
	Comment 9 • 7 years ago

Is Vinoth still working on this? Or its open for grab?

👁 Image	Vinoth
	Comment 10 • 7 years ago

Hello Arshad, yes I am.

👁 Image	Caitlin Neiman [:caitmuenster]
	Comment 11 • 7 years ago

Arshad, why don't you take a look at one of the unassigned bugs on this list? :) https://mzl.la/2yq1XA8

👁 Image	Arshad Kazmi [:arshadkazmi42]
	Comment 12 • 7 years ago

Thanks Caitlin. Will pick some unassigned bugs from the list

👁 Image	Caitlin Neiman [:caitmuenster]
	Comment 13 • 7 years ago

Hey Vinoth! Just wanted to check in and see how it is going with this bug. Let us know if you need any help getting unblocked!

👁 Image	Caitlin Neiman [:caitmuenster]
	Comment 14 • 7 years ago

Hey Vinoth, since we haven't heard from you in awhile, I'm going to unassign you from this bug and open it up for another contributor to work on.

👁 Image	Manuel Reimer
	Comment 15 • 7 years ago

Maybe you could also make "MAX_SESSION_RESULTS" a "getter" instead of a constant value and make it read the "browser.sessionstore.max_tabs_undo" preference, so it actually reflects the maximum possible items that could be returned by "getRecentlyClosed"? In fact it doesn't really make sense to hard-limit to 25 items in the "Firefox world". The maximum is already preconfigured to be only 10 items and if the user manually increases this, the larger number of items should also be available to extensions.

👁 Image	Caitlin Neiman [:caitmuenster]
	Updated • 6 years ago

👁 Image	Travis Virgil [:virgilt]
	Comment 16 • 6 years ago

Hi @caitmuenster can I work on this? Also general question: Am I limited to working on bugs listed as "good-first-bug" or can I attempt any bug listed on bugzilla?

👁 Image	Caitlin Neiman [:caitmuenster]
	Comment 17 • 6 years ago • Edited

Hey Travis, you can definitely work on this!

If you feel comfortable with the flow of fixing good-first-bugs, we'd love to see you branch out and take on harder bugs. In general, staff members tend to work on P1 and P2 bugs (usually because those patches are on the critical path and need to land before specific deadlines), and you should skip bugs that have assigned.

If you're looking to narrow down the list a bit, here's a query for current WebExtensions bugs: https://mzl.la/2KFryJa

If you want to work on a WebExtensions or Adds-on Manager bug, please let us know so we can keep an eye on incoming patches or help you out if you get stuck. :)

Edited to add: One more thing -- work on the WebExtensions Android component is suspended for the moment because we're in the middle of replacing our current Firefox for Android product.

👁 Image	Travis Virgil [:virgilt]
	Comment 18 • 6 years ago

Hi Caitlin,
I would love an opportunity to work on harder bugs for any project that could use my help, although I am trying to focus on WebExtensions and Adds-on Manager. I will ask about working on the bugs before I start. Thanks for answering my question and all the additional information!

👁 Image	Ajitesh13
	Comment 19 • 6 years ago

Hey! i would like to work in this bug. Is it still open ?

👁 Image	Caitlin Neiman [:caitmuenster]
	Comment 20 • 6 years ago

Hi Ajitesh, go for it! We'll assign you to the bug once you have a patch up on Phabricator. Same goes for the other bugs you commented on -- you might want to pick one to start. :) If you have any questions, please needinfo zombie (the bug's mentor).

👁 Image	Ajitesh13
	Comment 21 • 6 years ago

is the attacked test.xpi file corrupted ?

👁 Image	Ajitesh13
	Comment 22 • 6 years ago

attached (In reply to Ajitesh from comment #21)

is the attacked test.xpi file corrupted ?

attached*

👁 Image	Luca Greco [:rpl] [:luca] [:lgreco]
	Comment 23 • 6 years ago

(In reply to Ajitesh from comment #21)

is the attacked test.xpi file corrupted ?

No, the attached xpi file isn't corrupted, but it isn't signed and it doesn't contain (and it also doesn't contain an extension id in the manifest) and so to install it successfully you have to load it as a temporarily installed extension (e.g. from about:debugging)

👁 Image	arjunlalma
	Comment 24 • 6 years ago

Does that mean that the MAX_SESSION_RESULTS property should not be limited to just 25, rather it should be the maximum possible items that getRecentlyClosed() returns?

👁 Image	Kohei Yoshino [:kohei]
	Updated • 6 years ago

👁 Image	Luca Greco [:rpl] [:luca] [:lgreco]
	Updated • 6 years ago

👁 Image	Tomislav Jovanovic :zombie
	Comment 25 • 6 years ago

(In reply to arjunlalma from comment #24)

Does that mean that the MAX_SESSION_RESULTS property should not be limited to just 25, rather it should be the maximum possible items that getRecentlyClosed() returns?

This bug is about us not limiting the number of returned results to 25 when maxResults is not passed or undefined. The other approach of us intentionally not limiting to 25 results (because that's a limitation of Chrome that doesn't make as much sense for Firefox) would probably not be a [good first bug], so let's stick to the first approach.

👁 Image	kernp25
	Comment 26 • 4 years ago

👁 Image	Phabricator Automation
	Updated • 4 years ago

👁 Image	kernp25
	Comment 27 • 4 years ago

The real bug is that MAX_SESSION_RESULTS is always undefined.

The title of this bug should be: MAX_SESSION_RESULTS is ignored when not given maxResults because it is not defined

👁 Image	Rob Wu [:robwu]
	Comment 28 • 4 years ago

While this patch fixes the inconsistency in the API, it may make sense to change MAX_SESSION_RESULTS to a dynamic getter.

To extension developers following this: is there any use case for MAX_SESSION_RESULTS beyond 25?
If that is the case, please file a new bug report and link this one, and explain your use case.

👁 Image	Rob Wu [:robwu]
	Comment 29 • 4 years ago

The bug has apparently already been filed at bug 1764376; please add use cases there.

👁 Image	Pulsebot
	Comment 30 • 3 years ago

Pushed by rob@robwu.nl: https://hg.mozilla.org/integration/autoland/rev/8d3cd97887d1 Fix MAX_SESSION_RESULTS is not defined. r=robwu

👁 Image	Atila Butkovits
	Comment 31 • 3 years ago

Backed out for causing failures at browser_ext_sessions_forgetClosedTab.

Backout link: https://hg.mozilla.org/integration/autoland/rev/c748782f095840387c4d8564c15943f3b18209b3

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=8d3cd97887d1e8773c04d55b3cfc9c8c5fa81d79&selectedTaskRun=JPZ2QimyRw601vrqoEg9jQ.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=383982288&repo=autoland&lineNumber=24558

👁 Image	kernp25
	Comment 32 • 3 years ago

I think it makes more sense, if i fix bug 1764376 instead of this bug. So that extensions will not have a limit of 25.

Should this bug be closed?

👁 Image	Rob Wu [:robwu]
	Comment 33 • 3 years ago

(In reply to kernp25 from comment #32)

I think it makes more sense, if i fix bug 1764376 instead of this bug. So that extensions will not have a limit of 25.

Should this bug be closed?

If bug 1764376 gets fixed, then this bug could be fixed because of that.
The unit tests from this bug can be re-used / extended for that patch.

Would you be interested in picking up bug 1764376? You've already worked on the unit tests.

👁 Image	BugBot [:suhaib / :marco/ :calixte]
	Comment 34 • 3 years ago

This good-first-bug hasn't had any activity for 2 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.

👁 Image	BMO Automation
	Updated • 3 years ago

👁 Image	BugBot [:suhaib / :marco/ :calixte]
	Comment 35 • 3 years ago

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:kernp25, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit auto_nag documentation.

👁 Image	Rob Wu [:robwu]
	Comment 36 • 3 years ago

(In reply to kernp25 from comment #32)

I think it makes more sense, if i fix bug 1764376 instead of this bug. So that extensions will not have a limit of 25.

Should this bug be closed?

Nothing is happening in bug 1764376.
The current patch in this bug limits the maximum number of sessions to 25 (consistent with the API), but there is an expressed desire for returning more than a hard-coded 25 number of sessions.

Therefore this bug could be resolved by removing the condition + this.MAX_SESSION_RESULTS at https://searchfox.org/mozilla-central/rev/49011d374b626d5f0e7dc751a8a57365878e65f1/browser/components/extensions/parent/ext-sessions.js#137-138, optionally with // TODO bug 1764376: should be at most MAX_SESSION_RESULTS prepended. And the unit tests be updated to reflect that more than 25 sessions can be returned.

👁 Image	kernp25
	Comment 37 • 3 years ago

I added a patch in Bug 1764376. This bug can be closed?
When you are review the other patch?

👁 Image	Rob Wu [:robwu]
	Updated • 3 years ago

👁 Image	Amine Zroual Assignee
	Comment 38 • 2 months ago

The previous code branched on filter.maxResults == undefined and fell back to
this.MAX_SESSION_RESULTS, but that property never existed on this in this
scope (the schema-level constant lives in schemas/sessions.json, not on the JS
API object), so the branch silently resolved to undefined. slice(0, undefined)
on the inner helper then returned the full closed-sessions list anyway, making
the bug functionally a no-op but the code misleading.

Per comment 36, remove the conditional entirely and pass filter.maxResults
straight through. When it's undefined, slice(0, undefined) keeps doing the
right thing intentionally. Bug 1764376 tracks the separate question of whether
an explicit hard cap should exist at all.

The new test is a forward-looking pin per comment 36's instruction to reflect
that more than 25 sessions can be returned; pre-patch behavior was
accidentally-correct via slice(0, undefined), so the test is not a red->green
regression test for the removed branch but a guard against reintroducing a cap.

👁 Image	Phabricator Automation
	Updated • 2 months ago

👁 Image	Phabricator Automation
	Updated • 1 month ago

👁 Image	Pulsebot
	Comment 39 • 1 month ago

Pushed by rob@robwu.nl: https://github.com/mozilla-firefox/firefox/commit/792f7d27286f https://hg.mozilla.org/integration/autoland/rev/14a0f75fd7ff Default sessions.getRecentlyClosed() maxResults to Infinity. r=robwu

👁 Image	Pulsebot
	Comment 40 • 1 month ago

Pushed by agoloman@mozilla.com: https://github.com/mozilla-firefox/firefox/commit/28e38351cb8b https://hg.mozilla.org/integration/autoland/rev/755503d40a2a Revert "Bug 1392125 - Default sessions.getRecentlyClosed() maxResults to Infinity. r=robwu" for causing bc failures @browser_ext_sessions_getRecentlyClosed.js.

👁 Image	agoloman
	Comment 41 • 1 month ago

Backed out for causing bc failures @browser_ext_sessions_getRecentlyClosed.js.

• Backout link
• Push with failures
• Failure Log

👁 Image	Rob Wu [:robwu]
	Comment 42 • 1 month ago

The review helper bot already flagged why the test was failing: https://phabricator.services.mozilla.com/D293612#10246868

As a reviewer I didn't run the test locally because it looked plausible and I previously shared instructions on running tests after my initial review where I flagged an implementation issue. Please make sure that you run the tests at the next update of your patch.

👁 Image	Rob Wu [:robwu]
	Updated • 1 month ago

👁 Image	Pulsebot
	Comment 43 • 1 month ago

Pushed by rob@robwu.nl: https://github.com/mozilla-firefox/firefox/commit/3ad2bcff4200 https://hg.mozilla.org/integration/autoland/rev/a5d410f309a1 Default sessions.getRecentlyClosed() maxResults to Infinity. r=robwu

👁 Image	Norisz Fay [:noriszfay]
	Comment 44 • 1 month ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/a5d410f309a1