Closed Bug 1378647 Opened 7 years ago Closed 6 years ago

Support the "discarded" property inside browser.tabs.create()

Categories

(WebExtensions :: General, enhancement, P5)

Product:
WebExtensions
Component:
General
Type:
enhancement

Tracking

(firefox63 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: ntim, Assigned: mixedpuppy)

References

(Blocks 2 open bugs)

Details

(Keywords: dev-doc-complete, Whiteboard: triaged[sessions][design-decision-approved])

Attachments

(1 file, 2 obsolete files)

Bug 1378647 - support creating lazy tabs from extensions,
59 bytes, text/x-review-board-request
mikedeboer
: review+
rpl
: review+
JanH
: review+
Details 
Bug 1322485 adds support for tabs.discard, but doesn't allow creating unloaded tabs. It only allows creating a tab, then having it load until tabs.discard is called on that tab.


I would suggest a small API addition: supporting the discarded property in tabs.create. It's pretty consistent with the tabs API, because it's a property on the tab object: https://developer.chrome.com/extensions/tabs#type-Tab and tabs.create() actually allows a small subset of the tab object properties, it would just be a matter of expanding that subset to include `discarded`.
Tim, can you please describe a use case for creating a discarded tab?
Flags: needinfo?(ntim.bugs)
(In reply to Bob Silverberg [:bsilverberg] from comment #1)
> Tim, can you please describe a use case for creating a discarded tab?

If a session manager wants to restore a session with only the selected tab loaded, it would need a way to create discarded/unloaded tabs.
Flags: needinfo?(ntim.bugs)
Whiteboard: triaged[sessions][design-decision-needed]
Hi Tim, this bug will also be on the agenda for the July 18 WebExtensions APIs triage meeting. 

Wiki: https://wiki.mozilla.org/Add-ons/Contribute/Triage#Next_Meeting

Agenda: https://docs.google.com/document/d/1gWszBunGAyOJ_V8_HMECXJuZ4Gd_HTM_M7xjDSwSxeo/edit#
Priority: -- → P5
This add-on allows you to open links in a new unloaded (not loaded) tab.
Tab Mix Plus with 620k users also needs this API. Let me quite TMP help:
http://tabmixplus.org/support/viewtopic.php?t=3
>When opening many tabs from bookmarks group or history loading time for all tabs can be long, especially when multi-process (aka E10s, Electrolysis) is enabled. It is recommended to enable these options when opening many tabs. 
>Load tabs progressively - load tabs no more than 3 tabs at a time.
>Don't load tabs until selected - all background tabs remain unloaded until selected.
>For both options you can set the amount of tabs that serve as a threshold to enable the option.
It should look like this:
> browser.tabs.create({
>   url:"https://example.org",
>   discarded: true
> });
No concerns from the meeting, let's do it.
Whiteboard: triaged[sessions][design-decision-needed] → triaged[sessions][design-decision-approved]
Is the plan with this bug also to support the `Tab.discarded` and `tabs.discard(...)` APIs, or is that a separate issue?
(In reply to Michael Layzell [:mystor] from comment #10)
> Is the plan with this bug also to support the `Tab.discarded` and
> `tabs.discard(...)` APIs, or is that a separate issue?

This bug is about Tab.discarded and Bug 1322485 is about tabs.discard()
(In reply to Michael Layzell [:mystor] from comment #10)
> Is the plan with this bug also to support the `Tab.discarded` and
> `tabs.discard(...)` APIs, or is that a separate issue?

When Tab.discarded is supported you could realize tabs.discard() by closing the tab and reopening it with the same url, index and window as a discarded one. But of course then you loose the history.
Also those issues are close together. It absolutely would make sense to take care of them in one go.
(In reply to Tim Nguyen :ntim from comment #2)
> (In reply to Bob Silverberg [:bsilverberg] from comment #1)
> > Tim, can you please describe a use case for creating a discarded tab?
> 
> If a session manager wants to restore a session with only the selected tab
> loaded, it would need a way to create discarded/unloaded tabs.

Not to discourage implementing this bug, but I was thinking that for session manager stuff, why not open a bug to expose SessionStore.getBrowserState / SessionStore.setBrowserState et al?  It seems this would be a more direct approach overall to saving and restoring sessions.  Tabs would be restored in discarded state just as they are on startup.  IIRC, this was how Session Manager addon handled it legacy-wise.
(In reply to Kevin Jones from comment #13)
> (In reply to Tim Nguyen :ntim from comment #2)
> > (In reply to Bob Silverberg [:bsilverberg] from comment #1)
> > > Tim, can you please describe a use case for creating a discarded tab?
> > 
> > If a session manager wants to restore a session with only the selected tab
> > loaded, it would need a way to create discarded/unloaded tabs.
> 
> Not to discourage implementing this bug, but I was thinking that for session
> manager stuff, why not open a bug to expose SessionStore.getBrowserState /
> SessionStore.setBrowserState et al?  It seems this would be a more direct
> approach overall to saving and restoring sessions.  Tabs would be restored
> in discarded state just as they are on startup.  IIRC, this was how Session
> Manager addon handled it legacy-wise.

FWIW, I have filed bug 1413525.
I am not in a place to take on this bug right now, but I thought I would offer some input.

We don't currently have an API for directly opening a tab with restore state (eg, url) in the discarded state.  In tabbrowser, only blank tabs are opened in the discarded state (lazy).

When a session is restored, associating the restore data with a lazy/discarded tab is handled in SessionStore, which is held in TabStateCache for retrieval upon de-lazifying the tab.

One way to deal with this could be something like:

let lazyTabState = {entries: [{url: "http://example.com/", ...}]};
let tab = gBrowser.addTab("about:blank", {createLazyBrowser: true});
SessionStore.setTabState(tab, JSON.stringify(lazyTabState));

(as I have done in similarly in browser/components/extensions/test/browser/browser_ext_tabs_discarded.js)

This might be simpler than mucking with SessionStore code.
I read your note about the lack of the API for this functionality, so wanted to add a comment I had added to another bug explaining a use case for this API to be recreated for current firefox.

Using portions of the API that were not completed in the latest version, a user could use firefox as a tool, similar to word or excel.  We could:
 - open a series of tabs and work on them over time
 - if we closed a tab accidentally, we could reopen the tab
 - if we closed a window and needed something in that window, we could reopen that window
 - if we had an important session, we could save it to our history for recovery later

In general, this functionality makes firefox so useful, it is hard to imagine not having this functionality.
Tab Session Manager now uses a workaround: it loads a custom page with just a title, a favicon and a URL to which the page is redirected once the tabs is active. See https://github.com/sienori/Tab-Session-Manager/blob/9697e3fc322844aafc277a947bac8fcd6e4d9adf/Tab-Session-Manager/background.js#L407
See Also: → 1422588
Voting for implementation of the "discarded" property as proposed.


Speaking as a user with developer background:

My working session currently consists of about 150 tabs, which I want to "lazily" restore after restarting the browser. Before Firefox 57 introduced the new API, this was not an issue at all, and went quickly and smoothly.

Using Firefox 57, no matter which session manager I tried, I was surprised to see every single tab being actually loaded when restoring the session. Not only this is taking ages, I even had to extend my VM's main memory since Firefox was hitting the limit during that action.

I was even more surprised when now reading that so to say "lazy session restore" currently can't be supported because of missing functionality in the API.
Blocks: Session_managers
Just to make sure that you know about OTHER SIMILAR BUGS


The list of APIs/bugs related to session managers:

(!)  Bug 1427928 (Session_managers) - meta-bug with purpose to group all bugs related to missing and available APIs needed by session managers (new bugs will be linked to this meta-bug – in the future CHECK IN SECTION “Tracking/Depends on” located at the top of page). 
It can be also the place to discuss about the best way to support session managers with APIs, and about new/potential APIs/bug fixes needed, but not filed yet in Bugzilla.

(!)  Bug 1378647 – Support the "discarded" property inside browser.tabs.create()
(!)  Bug 1378651 – Allow accessing back-forward history for each tab
(!)  Bug 1381922 – Allow modifying/restoring back-forward history for each tab
(!)  Bug 1413525 – Expose SessionStore.getBrowserState and SessionStore.setBrowserState

Bug 1422588, Bug 1421254 – other bugs


Meta-bugs related to Session Restore:

Bug 1330633 (ss-reliability) - Sessionstore reliability tracking
Bug 1330635 (ss-perf) - Sessionstore performance tracking
Bug 1330638 (ss-feature) - Sessionstore feature development
Bug 450886 - [meta] restore window features as accurately as possible


(please, consider to support these bugs: vote on them or discuss specific information that will help to resolve this bugs)
(In reply to u462496 from comment #15)
> We don't currently have an API for directly opening a tab with restore state
> (eg, url) in the discarded state.  In tabbrowser, only blank tabs are opened
> in the discarded state (lazy).

This seemed like a bug to me, I filed bug 1446426.

> One way to deal with this could be something like:
> 
> let lazyTabState = {entries: [{url: "http://example.com/", ...}]};
> let tab = gBrowser.addTab("about:blank", {createLazyBrowser: true});
> SessionStore.setTabState(tab, JSON.stringify(lazyTabState));
> 
> (as I have done in similarly in
> browser/components/extensions/test/browser/browser_ext_tabs_discarded.js)

Nice trick! But I think gBrowser.addTab should do something like this automatically when createLazyBrowser is true.
Blocks: tab-unloading
No longer blocks: Session_managers
Blocks: Session_managers
I'd like to be able to set favicon and title as well, otherwise tab has default icon and title is taken from url.

> browser.tabs.create({
>   url:"https://example.org",
>   discarded: true,
>   title: "my title",
>   favIconUrl: "my url"
> });

This also should be considered:
> browser.windows.create({
>    url: [],
>    title: [],
>    discarded: true
> });
Second thing is not that important because we actually can create window, and create discarded tabs afterwards.
Product: Toolkit → WebExtensions
Shane Caraveo is working on bug 1378647:
https://bugzilla.mozilla.org/show_bug.cgi?id=1422588#c24

THANK YOU
Assignee: nobody → mixedpuppy
Comment on attachment 8988302 [details]
Bug 1378647 - support creating lazy tabs from extensions,

The original patch had changes in tabbrowser, but they were not necessary.  removing dao (though you can take a look if you want).
Attachment #8988302 - Flags: review?(dao+bmo)
Comment on attachment 8988302 [details]
Bug 1378647 - support creating lazy tabs from extensions,

https://reviewboard.mozilla.org/r/253568/#review260146

::: browser/components/extensions/schemas/tabs.json:590
(Diff revision 1)
> +              "discard": {
> +                "type": "boolean",
> +                "optional": true,
> +                "description": "Whether the tab is created."
> +              },

Can we use `discarded` instead of `discard` ? That way, we're compatible with chrome.
(In reply to Tim Nguyen :ntim from comment #31)
> Comment on attachment 8988302 [details]

> Can we use `discarded` instead of `discard` ? That way, we're compatible
> with chrome.

Is it actually supported on chrome?  It's not documented at https://developer.chrome.com/extensions/tabs#method-create
Flags: needinfo?(ntim.bugs)
I would use 'discarded' for consistency with Tab.discarded, even if Chrome doesn't support the parameter.
(In reply to Oriol Brufau [:Oriol] from comment #33)
> I would use 'discarded' for consistency with Tab.discarded, even if Chrome
> doesn't support the parameter.

That's fine, but I want to know that if chrome does implement it, that it is indeed "discarded".
(In reply to Shane Caraveo (:mixedpuppy) from comment #32)
> Is it actually supported on chrome?  It's not documented at
> https://developer.chrome.com/extensions/tabs#method-create

Ah, I confused it with the property on the Tab type: https://developer.chrome.com/extensions/tabs#type-Tab

Still, I agree with :Oriol about consistency.
Flags: needinfo?(ntim.bugs)
Comment on attachment 8988302 [details]
Bug 1378647 - support creating lazy tabs from extensions,

https://reviewboard.mozilla.org/r/253568/#review263180

Hi Shane, sorry for letting you wait on this one.

The implementation looks pretty simple and straightfoward (I've just some minor notes or issues here and there, and some doubts related to the default behavior when `active` is not esplicitly passed as `true` in the `tabs.create` options).

My main concerns are about that `JSON.stringify` that we are currently using on the `SessionStore.setTabState` call (which makes me wondering if we should actually makes the "creation of a discarded tab" a more "official feature" of the API provided by the underlying layers, tabbrowser and/or `SessionStore`) and the issues related to the interactions between the creation of the discarded tabs and the `tabs.executeScript` calls described below in a bit more details.

::: browser/components/extensions/parent/ext-tabs.js:589
(Diff revision 2)
>                  return Promise.reject({message: "Opener tab must be in the same window as the tab being created"});
>                }
>              }
>  
> -            if (createProperties.index != null) {
> -              options.index = createProperties.index;
> +            // Simple properties
> +            const properties = ["index", "pinned", "title"];

should we reject if the API call has received a `title` option for a non-discarded tab?

::: browser/components/extensions/parent/ext-tabs.js:589
(Diff revision 2)
>                  return Promise.reject({message: "Opener tab must be in the same window as the tab being created"});
>                }
>              }
>  
> -            if (createProperties.index != null) {
> -              options.index = createProperties.index;
> +            // Simple properties
> +            const properties = ["index", "pinned", "title"];

In Bug 1446426 was mentioned that we were also thinking to allow to provide a favicon for the newly created discarded tab, is it still something that we plan to do? (maybe in a follow up?)

::: browser/components/extensions/parent/ext-tabs.js:596
(Diff revision 2)
>              let active = true;
>              if (createProperties.active !== null) {
>                active = createProperties.active;
>              }

This patch currently adds `"default": true` on the API metadata related to the `active` option, and so it seems that `createProperties.active` cannot be `null` anymore (and so it seemse that this could become just `let active = createProperties.active`).

::: browser/components/extensions/parent/ext-tabs.js:600
(Diff revision 2)
> +            if (createProperties.discarded) {
> +              if (active) {
> +                return Promise.reject({message: `Active tabs cannot be created and discarded.`});
> +              }
> +              if (createProperties.pinned) {
> +                return Promise.reject({message: `Pinned tabs cannot be created and discarded.`});
> +              }
> +              if (!discardable) {
> +                return Promise.reject({message: `Cannot create a discarded new tab, about or data urls.`});
> +              }
> +              options.createLazyBrowser = true;
> +            }

None of this part and the `discardable` variable defined at line 568 seems to really need to wait for the `window` object that it is "discovered" by the Promise defined between 503-520, and so I'm wondering if it could be worth to check these error conditions in the start of the `create` API method and return earlier the `Promise.reject` results.

(Not marking this as an issue, because it isn't actually a real issue)

::: browser/components/extensions/parent/ext-tabs.js:601
(Diff revision 2)
>              let active = true;
>              if (createProperties.active !== null) {
>                active = createProperties.active;
>              }
> +            if (createProperties.discarded) {
> +              if (active) {

I'm wondering if it would make sense to not set the `"default"` for `active` in the API metadata, then only reject if active has been explicitly passed as `true` by the extension, and consider `active` to default to `false` if `createProperties.discarded` is `true` and `createProperties.active === null`.

Otherwise, it basically means that `discarded` can never be passed alone, as in:

`browser.tabs.create({url: "...", discarded: true});`

but it has to always be explictly paired with `active: false`, as in:

`browser.tabs.create({url: "...", active: false, discarded: true});`

Anyway, I don't have a super-strong opinion on it, but it seems worth to mention it now. 

Besides which is the final behavior that we agreed on, we should not forget to highlight it in the updates to the API reference on MDN.

::: browser/components/extensions/parent/ext-tabs.js:615
(Diff revision 2)
> +              SessionStore.setTabState(nativeTab, JSON.stringify({
> +                entries: [{
> +                  url: url,
> +                  title: options.title,
> +                  triggeringPrincipal_base64: Utils.serializePrincipal(context.principal),
> +                }],
> +              }));

It looks that here we are serializing the "session tab state" of the discarded tab from its json representation into a string, and then the first thing that `SessionStore.setTabState` is going to do internally is (obviously) to deserialize the string back into an object :-(
(https://searchfox.org/mozilla-central/rev/a80651653faa78fa4dfbd238d099c2aad1cec304/browser/components/sessionstore/SessionStore.jsm#2388)

What concerns me a bit about this is that it makes it feels like we are using `SessionStore.setTabState` in a way that may not be its initially intended usage ("restoring tabs from a stored session"), and so I'm wondering if it may also have (now, or over the time) some additional side-effects that we may not see right now.

We should double-check (at least if we didn't already) with who works on the `SessionStore`:

- if there are any side-effects of `SessionStore.setTabState` that we may need to take into account

- if we can expose from SessionStore a method which would allow us to set the tab state without serializing the state into a string.

I noticed that initially we were thinking of exposing the changes needed directly into the `tabbrowser`'s `addTab` method in Bug 1446426, then we changed plans to not mess too much with `addTab`.

How about a new `gBrowser.addDiscardedTab` method instead? (which would call addTab internally, and then set the tab state on it).

::: browser/components/extensions/parent/ext-tabs.js:642
(Diff revision 2)
>  
>                // Mark the tab as initializing, so that operations like
>                // `executeScript` wait until the requested URL is loaded in
>                // the tab before dispatching messages to the inner window
>                // that contains the URL we're attempting to load.
>                tabListener.initializingTabs.add(nativeTab);

So... it looks that the newly create tabs are being added to the `initializingTabs` so that calls to `tabs.executeScript` would wait until the tab is ready... which pushed me to think a bit more about what that would means when the tabs are actually discarded, e.g. 

a "session manager" extension creates a number of discarded tabs (a restored session) and hides them using the tab hiding API, while another extension (e.g. a userScript manager like Greasemonkey) is listening to the `tabs.onCreated` and on every newly created tab it calls `tabs.executeScript` on it, trying to run a script when the document in the tab is starting to load.

All those tabs.executeScript becomes pending promises that are never resolved if the user never selects those tabs.

Once the user selects the tab (which may be after a good amount of time from when the other extension called `tabs.executeScript` on it), the "queued" `tabs.executeScript` calls are going to be executed. 
At this point the discarded tab seems to load "about:blank" first, and so if the `tabs.executeScript` call doesn't include `matchAboutBlank: true` in the options, the promise returned by `tabs.executeScript` rejects with "Missing host permission for the tab" (which will be probably surprising because the tab is going to load the final url, and an extension which have the right origin permissions will still see that error message).

If the tabs.executeScript call contains `matchAboutBlank: true` is even worst, because the injected script will match "about:blank" but gets stuck indefinitely in Script.injectInto, waiting "about:blank" to become idle, which is never going to happen because that document is immediately navigated to the url set on the discarded tab.

I'm thinking that it may be more reasonable to make `promiseTabWhenReady` or `tabListener.awaitTabReady` to reject immediately on discarded tabs, instead of waiting potentially forever.

(I guess that this issue, or part of it, is already happening, e.g. when a session is restored and/or with tabs discarded from the extensions that are using tabs.discard).

::: browser/components/extensions/test/browser/browser_ext_tabs_discarded.js:105
(Diff revision 2)
> +        browser.tabs.remove(details.tabId);
> +        browser.test.notifyPass("test-finished");
> +      }, {url: [{hostContains: "example.com"}]});
> +
> +      browser.tabs.onCreated.addListener(tab => {
> +        browser.test.assertEq(tabOpts.url, tab.url, "lazy tab url is correct");

How about also asserting here that:
- tab.active is false
- tab.discarded is true
Attachment #8988302 - Flags: review?(lgreco)
Blocks: 1475240
(In reply to Luca Greco [:rpl] from comment #37)
> Comment on attachment 8988302 [details]
> Bug 1378647 - support creating lazy tabs from extensions,
> 
> https://reviewboard.mozilla.org/r/253568/#review263180
> 
> ...
> If the tabs.executeScript call contains `matchAboutBlank: true` is even
> worst, because the injected script will match "about:blank" but gets stuck
> indefinitely in Script.injectInto, waiting "about:blank" to become idle,
> which is never going to happen because that document is immediately
> navigated to the url set on the discarded tab.
> 
> I'm thinking that it may be more reasonable to make `promiseTabWhenReady` or
> `tabListener.awaitTabReady` to reject immediately on discarded tabs, instead
> of waiting potentially forever.
> 
> (I guess that this issue, or part of it, is already happening, e.g. when a
> session is restored and/or with tabs discarded from the extensions that are
> using tabs.discard).

This is interesting. So does that mean that userscripts (and other extensions) will not work/activate on tabs that are loaded (after having sat in a discarded state for a while)? I see this sometimes in browsers, extensions not working in tabs that were initiated as unloaded and then later activated when the user clicks them. Or is this what you're trying to fix? Sorry, I'm a total layman. Especially for extensions like Noscript, Ublock, Umatrix, this could result in privacy and/or security leaks. Or are they not affected, only userscripts? Or perhaps I misunderstood the issue completely.
Comment on attachment 8988302 [details]
Bug 1378647 - support creating lazy tabs from extensions,

https://reviewboard.mozilla.org/r/253568/#review263180

> should we reject if the API call has received a `title` option for a non-discarded tab?

sure

> In Bug 1446426 was mentioned that we were also thinking to allow to provide a favicon for the newly created discarded tab, is it still something that we plan to do? (maybe in a follow up?)

followup dependency was added.

> This patch currently adds `"default": true` on the API metadata related to the `active` option, and so it seems that `createProperties.active` cannot be `null` anymore (and so it seemse that this could become just `let active = createProperties.active`).

Yeah, after looking through this, I decided to do something a little different.

> None of this part and the `discardable` variable defined at line 568 seems to really need to wait for the `window` object that it is "discovered" by the Promise defined between 503-520, and so I'm wondering if it could be worth to check these error conditions in the start of the `create` API method and return earlier the `Promise.reject` results.
> 
> (Not marking this as an issue, because it isn't actually a real issue)

Lets leave a refactoring of this function for a different time.

> It looks that here we are serializing the "session tab state" of the discarded tab from its json representation into a string, and then the first thing that `SessionStore.setTabState` is going to do internally is (obviously) to deserialize the string back into an object :-(
> (https://searchfox.org/mozilla-central/rev/a80651653faa78fa4dfbd238d099c2aad1cec304/browser/components/sessionstore/SessionStore.jsm#2388)
> 
> What concerns me a bit about this is that it makes it feels like we are using `SessionStore.setTabState` in a way that may not be its initially intended usage ("restoring tabs from a stored session"), and so I'm wondering if it may also have (now, or over the time) some additional side-effects that we may not see right now.
> 
> We should double-check (at least if we didn't already) with who works on the `SessionStore`:
> 
> - if there are any side-effects of `SessionStore.setTabState` that we may need to take into account
> 
> - if we can expose from SessionStore a method which would allow us to set the tab state without serializing the state into a string.
> 
> I noticed that initially we were thinking of exposing the changes needed directly into the `tabbrowser`'s `addTab` method in Bug 1446426, then we changed plans to not mess too much with `addTab`.
> 
> How about a new `gBrowser.addDiscardedTab` method instead? (which would call addTab internally, and then set the tab state on it).

Yeah, kind of lame.  I'll fix SessionStore and get an additional review.  This is likely the only place we'll create discarded tabs other than via session restore, I think it's fine to handle this from our side.  The test will catch any problem if there are changes in session store.

> So... it looks that the newly create tabs are being added to the `initializingTabs` so that calls to `tabs.executeScript` would wait until the tab is ready... which pushed me to think a bit more about what that would means when the tabs are actually discarded, e.g. 
> 
> a "session manager" extension creates a number of discarded tabs (a restored session) and hides them using the tab hiding API, while another extension (e.g. a userScript manager like Greasemonkey) is listening to the `tabs.onCreated` and on every newly created tab it calls `tabs.executeScript` on it, trying to run a script when the document in the tab is starting to load.
> 
> All those tabs.executeScript becomes pending promises that are never resolved if the user never selects those tabs.
> 
> Once the user selects the tab (which may be after a good amount of time from when the other extension called `tabs.executeScript` on it), the "queued" `tabs.executeScript` calls are going to be executed. 
> At this point the discarded tab seems to load "about:blank" first, and so if the `tabs.executeScript` call doesn't include `matchAboutBlank: true` in the options, the promise returned by `tabs.executeScript` rejects with "Missing host permission for the tab" (which will be probably surprising because the tab is going to load the final url, and an extension which have the right origin permissions will still see that error message).
> 
> If the tabs.executeScript call contains `matchAboutBlank: true` is even worst, because the injected script will match "about:blank" but gets stuck indefinitely in Script.injectInto, waiting "about:blank" to become idle, which is never going to happen because that document is immediately navigated to the url set on the discarded tab.
> 
> I'm thinking that it may be more reasonable to make `promiseTabWhenReady` or `tabListener.awaitTabReady` to reject immediately on discarded tabs, instead of waiting potentially forever.
> 
> (I guess that this issue, or part of it, is already happening, e.g. when a session is restored and/or with tabs discarded from the extensions that are using tabs.discard).

I think this is a seperate bug as it would already be a problem with any discarded tab, whether via session restore or tabs.discard.  Possibly bug 1464992.
Attachment #8988302 - Attachment is obsolete: true
Attachment #8991686 - Attachment is obsolete: true
Attachment #8991686 - Flags: review?(mdeboer)
Attachment #8991686 - Flags: review?(lgreco)
r? @mdeboer for sessionstore changes and use.
Comment on attachment 8991689 [details]
Bug 1378647 - support creating lazy tabs from extensions,

https://reviewboard.mozilla.org/r/256628/#review263634

r=me with the comments addressed.

::: browser/components/extensions/schemas/tabs.json:592
(Diff revision 1)
> +                "description": "Whether the document in the tab should be opened in reader mode."
> +              },
> +              "discarded": {
> +                "type": "boolean",
> +                "optional": true,
> +                "description": "Whether the tab is discarded when created."

"Whether the tab is marked as 'discarded' when created."

::: browser/components/sessionstore/SessionStore.jsm:2391
(Diff revision 1)
>      // Note that we cannot simply replace the contents of the cache
>      // as |aState| can be an incomplete state that will be completed
>      // by |restoreTabs|.
> -    let tabState = JSON.parse(aState);
> +    let tabState = aState;
> +    if (typeof tabState != "object") {
> +      tabState = JSON.parse(aState);

I think you're placing the closing brace down a bit too aggressively. I'm ok with the change if you change this to:

```js
if (typeof tabState == "string") {
  tabState = JSON.parse(aState);
}
```

And leave the rest as-is.
Comment on attachment 8991689 [details]
Bug 1378647 - support creating lazy tabs from extensions,

https://reviewboard.mozilla.org/r/256628/#review263638
Attachment #8991689 - Flags: review?(mdeboer) → review+
Comment on attachment 8991689 [details]
Bug 1378647 - support creating lazy tabs from extensions,

https://reviewboard.mozilla.org/r/256628/#review263668

::: browser/components/extensions/parent/ext-tabs.js:570
(Diff revision 1)
>  
> +            // Only set disallowInheritPrincipal on non-discardable urls as it
> +            // will override creating a lazy browser.  Setting triggeringPrincipal
> +            // will ensure other cases are handled, but setting it may prevent
> +            // creating about and data urls.
> +            let discardable = url && !url.startsWith("about:") && !url.startsWith("data:");

I'm pretty sure that "data:" urls should actually never reach this part of tabs.create and be actually rejected from line 529 with the error "Illegal URL: ..." (along with "chrome:", "javascript:", "file:" and privileged "about:" URLS).

And so it may not be needed to also special case "data:" urls explictly here (but we definitely need to still special case the "about:" urls, because only the privileged ones are going to be rejected from line 529).

::: browser/components/extensions/parent/ext-tabs.js:621
(Diff revision 1)
> -
> -            let active = true;
> -            if (createProperties.active !== null) {
> -              active = createProperties.active;
> +            if (createProperties.discarded) {
> +              SessionStore.setTabState(nativeTab, {
> +                entries: [{
> +                  url: url,
> +                  title: options.title,
> +                  triggeringPrincipal_base64: Utils.serializePrincipal(context.principal),

It would be nice if we could also avoid to serialize the principal here (it seems that it would be deserialized here: https://searchfox.org/mozilla-central/rev/46292b1212d2d61d7b5a7df184406774727085b8/toolkit/modules/sessionstore/SessionHistory.jsm#447-449).

It would also allow us to remove the `Utils` module getter added at line 17 by this patch.

::: browser/components/extensions/test/browser/browser_ext_tabs_discarded.js:35
(Diff revision 1)
>    });
>  
>    await extension.startup();
>  
> -  let tab1 = await BrowserTestUtils.openNewForegroundTab(gBrowser, "http://example.com");
> -
> +  let testTab = BrowserTestUtils.addTab(gBrowser, "about:blank", {createLazyBrowser: true});
> +  SessionStore.setTabState(testTab, JSON.stringify(lazyTabState));

Nit, it seems that with the change applied to `SessionStore.setTabState` we can avoid to stringify the lazyTabState here too.
Attachment #8991689 - Flags: review?(lgreco)
Comment on attachment 8991689 [details]
Bug 1378647 - support creating lazy tabs from extensions,

https://reviewboard.mozilla.org/r/256628/#review263668

> It would be nice if we could also avoid to serialize the principal here (it seems that it would be deserialized here: https://searchfox.org/mozilla-central/rev/46292b1212d2d61d7b5a7df184406774727085b8/toolkit/modules/sessionstore/SessionHistory.jsm#447-449).
> 
> It would also allow us to remove the `Utils` module getter added at line 17 by this patch.

I'd prefer to avoid much more sessionstore refactoring in this patch.  The json item was simple, but this might (tbh haven't looked) get into changing the structure of the object/etc.

> Nit, it seems that with the change applied to `SessionStore.setTabState` we can avoid to stringify the lazyTabState here too.

You may have reviewed an earlier version, the last version does not stringify.
Comment on attachment 8991689 [details]
Bug 1378647 - support creating lazy tabs from extensions,

https://reviewboard.mozilla.org/r/256628/#review264710

Looks good to me, thanks Shane.

It would be nice to also test explictly the new expected rejections (e.g. using title without discarded, using discarded with an about:blank url, and with pinned or active set explicitly to true).

Have you already filed a follow up for the additional changes needed to SessionStore.setTabState to avoid to serialize principal?
Attachment #8991689 - Flags: review?(lgreco) → review+
See Also: → 1476616
Keywords: dev-doc-needed
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/6cc8354ce3c5
support creating lazy tabs from extensions, r=mikedeboer,rpl
Comment on attachment 8991689 [details]
Bug 1378647 - support creating lazy tabs from extensions,

asking for a refresh on review due to android changes.
Flags: needinfo?(mixedpuppy)
Attachment #8991689 - Flags: review?(lgreco)
Comment on attachment 8991689 [details]
Bug 1378647 - support creating lazy tabs from extensions,

https://reviewboard.mozilla.org/r/256628/#review267050

::: mobile/android/chrome/content/browser.js:3775
(Diff revision 6)
> +    let triggeringPrincipal_base64 = aParams.triggeringPrincipal ?
> +      Utils.serializePrincipal(aParams.triggeringPrincipal) : Utils.SERIALIZED_SYSTEMPRINCIPAL;
>      this.browser.__SS_data = {
>        entries: [{
>          url: uri,
>          title: truncate(title, MAX_TITLE_LENGTH),
> -        triggeringPrincipal_base64: Utils.SERIALIZED_SYSTEMPRINCIPAL
> +        triggeringPrincipal_base64,

The changes in this files looks reasonable to me, but do you mind to also ask an additional sign-off restricted to the changes on this file? (e.g. :JanH seems to have reviewed other similar changes on this file in the past).

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_basic.html:310
(Diff revision 6)
>        await browser.tabs.remove(tabId);
>        browser.test.sendMessage("done");

Nit, it looks that this is what is supposed to run when the "done" test message is sent to this extension and so (now that we are also sending a "create" message to it), it  may be a good idea to  check that msg is "done" also for this case.
Comment on attachment 8991689 [details]
Bug 1378647 - support creating lazy tabs from extensions,

Setting the r+ on Bugzilla (with the notes from Comment 56, the one set from mozreview didn't update the one on bugzilla accordingly)
Attachment #8991689 - Flags: review?(lgreco) → review+
Comment on attachment 8991689 [details]
Bug 1378647 - support creating lazy tabs from extensions,

Jan, can you take a look at the triggeringPrincipal changes in android browser.js?
Attachment #8991689 - Flags: review?(jh+bugzilla)
Comment on attachment 8991689 [details]
Bug 1378647 - support creating lazy tabs from extensions,

https://reviewboard.mozilla.org/r/256628/#review267382

The mobile/* changes look fine, I guess.
Attachment #8991689 - Flags: review?(jh+bugzilla) → review+
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/fb52c3f74edc
support creating lazy tabs from extensions, r=JanH,mikedeboer,rpl
https://hg.mozilla.org/mozilla-central/rev/fb52c3f74edc
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
No longer blocks: Session_managers
Is manual QA needed on this bug? If yes, please provide a test webextension and some STRs, else add the "qe-verify-" flag.
Flags: needinfo?(mixedpuppy)
Flags: needinfo?(mixedpuppy) → qe-verify-

Compatibility data was already in place, the tab.create page has been updated to include descriptions of discarded and title. Can you please review the new content and let me know if any changes are needed.

Flags: needinfo?(mixedpuppy)

lgtm

Flags: needinfo?(mixedpuppy)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: