Open Bug 1392125 Opened 7 years ago Updated 11 months ago

Omitted maxResults property not handled correctly in getRecentlyClosed

Categories

(WebExtensions :: General, defect, P5)

Product:
WebExtensions
Component:
General
Version:
52 Branch
Type:
defect

Tracking

(firefox-esr52 affected)

Tracking Flags:
Tracking Status
firefox-esr52 --- affected

People

(Reporter: yfdyh000, Unassigned, Mentored, NeedInfo)

References

(Blocks 1 open bug)

Details

(Keywords: good-first-bug, Whiteboard: [history])

Attachments

(2 files)

test.xpi
577 bytes, application/x-xpinstall
Details
Bug 1392125 - Fix MAX_SESSION_RESULTS is not defined. r?zombie
48 bytes, text/x-phabricator-request
Details | Review
Attached file test.xpi — 
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.
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.
Has STR: --- → yes
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.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
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.
Flags: needinfo?(kmaglione+bmo)
> 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.
(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.
Status: RESOLVED → REOPENED
Flags: needinfo?(kmaglione+bmo)
Resolution: WONTFIX → ---
Summary: MAX_SESSION_RESULTS is ignored when not given maxResults → Omitted maxResults property not handled correctly in getRecentlyClosed
Mentor: bob.silverberg
status-firefox57: --- → wontfix
Priority: -- → P5
Whiteboard: [good first bug] → [good first bug][history]
Product: Toolkit → WebExtensions
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?
(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
Assignee: nobody → vinoth.maruthalingam
Mentor: bob.silverberg → tomica
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. :)
Is Vinoth still working on this? Or its open for grab?
Flags: needinfo?(vinoth.maruthalingam)
Flags: needinfo?(tomica)
Flags: needinfo?(cneiman)
Hello Arshad, yes I am.
Flags: needinfo?(vinoth.maruthalingam)
Arshad, why don't you take a look at one of the unassigned bugs on this list? :) https://mzl.la/2yq1XA8
Flags: needinfo?(tomica)
Flags: needinfo?(cneiman)
Thanks Caitlin. Will pick some unassigned bugs from the list
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!
Flags: needinfo?(vinoth.maruthalingam)
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.
Assignee: vinoth.maruthalingam → nobody
Flags: needinfo?(vinoth.maruthalingam)
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.

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?

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.

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!

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

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).

is the attacked test.xpi file corrupted ?

attached (In reply to Ajitesh from comment #21)

is the attacked test.xpi file corrupted ?

attached*

(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)

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?

Whiteboard: [good first bug][history] → [history]
Flags: needinfo?(tomica)

(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.

Flags: needinfo?(tomica)
Assignee: nobody → kernp25

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

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.

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

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

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?

Flags: needinfo?(kernp25) → needinfo?(rob)

(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.

Flags: needinfo?(rob)

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

Assignee: kernp25 → nobody
Status: REOPENED → NEW
Severity: minor → S4

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.

Flags: needinfo?(rob)
Flags: needinfo?(kernp25)

(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.

Flags: needinfo?(rob)

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

Flags: needinfo?(kernp25) → needinfo?(rob)
See Also: → 1836877
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: