Open Bug 1420681 Opened 7 years ago Updated 2 years ago

Add forceDiscard property to tabs.discard

Categories

(WebExtensions :: General, enhancement, P3)

Product:
WebExtensions
Component:
General
Type:
enhancement

Tracking

(Not tracked)

People

(Reporter: u462496, Unassigned)

References

Details

Attachments

(2 files)

add_forceDiscard_property_to_tabs.discard.diff
8.86 KB, patch
mixedpuppy
: feedback+
Details | Diff | Splinter Review
Bug 1420681 - add forceDiscard property to tabs.discard - V4
59 bytes, text/x-review-board-request
Details 
tabbrowser.discardBrowser now has forceDiscard property to override blocking of beforeunload handlers.

Lets allow extensions to give permission to take advantage of this.

While one could argue that setting `dom.disable_beforeunload = true` would accomplish the same thing, some users (myself included) may not necessarily want to override beforeunload handlers on all tabs globally, yet have the option to override for discarding.  Also, the forceDiscard property in discardBrowser still allows beforeunload handlers to run, it just overrides blocking prompts.  I am not sure if `dom.disable_beforeunload` does the same.
Attachment #8931917 - Flags: feedback?(mixedpuppy)
I would like to add that I have tested, and Google Chrome forces discarding of tabs with blocking beforeunload handlers without warning.

Bug 1415918 may get uplifted to 58, and it would be good if this could get landed and be uplifted also so the API debuts complete.
(In reply to Kevin Jones from comment #0)
> ...the forceDiscard property in
> discardBrowser still allows beforeunload handlers to run, it just overrides
> blocking prompts.  I am not sure if `dom.disable_beforeunload` does the same.

Sorry for my mistake, yes `dom.disable_beforeunload` does allow all beforeunload to be fired.
Comment on attachment 8931917 [details] [diff] [review]
add_forceDiscard_property_to_tabs.discard.diff

Please start using reviewboard.

Aside from the feedback below, I want to understand the potential problems with allowing extensions to force discard.  Can you outline the behavior?

>diff --git a/browser/components/extensions/schemas/tabs.json b/browser/components/extensions/schemas/tabs.json
>--- a/browser/components/extensions/schemas/tabs.json
>+++ b/browser/components/extensions/schemas/tabs.json
>@@ -887,16 +887,28 @@

>+          {
>+            "type": "object",
>+            "name": "discardProperties",
>+            "optional": true,
>+            "properties": {
>+              "forceDiscard": {
>+                "type": "boolean",
>+                "optional": true,
>+                "description": "Forces discard regarless of whether an unload handler wants to prompt."
>+              }
>+            }

Does this present prompts?  Or does it prevent prompts?
Document the default is false?  
s/regarless/regardless/

>diff --git a/browser/components/extensions/test/browser/browser-common.ini b/browser/components/extensions/test/browser/browser-common.ini
>--- a/browser/components/extensions/test/browser/browser-common.ini
>+++ b/browser/components/extensions/test/browser/browser-common.ini
>@@ -6,16 +6,17 @@ support-files =

>+  file_discardBrowser.html

lets make it file_onbeforeunload.html


>diff --git a/browser/components/extensions/test/browser/browser_ext_tabs_discard.js b/browser/components/extensions/test/browser/browser_ext_tabs_discard.js
>--- a/browser/components/extensions/test/browser/browser_ext_tabs_discard.js
>+++ b/browser/components/extensions/test/browser/browser_ext_tabs_discard.js
>@@ -1,62 +1,80 @@
> /* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */
> /* vim: set sts=2 sw=2 et tw=80: */
> /* global gBrowser SessionStore */
> "use strict";
> 
>+const BASE_URL = "http://mochi.test:8888/browser/browser/components/extensions/test/browser/";

I know we do this everywhere, but I think you should be able to just use SimpleTest.getTestFileURL("filename.html");
Attachment #8931917 - Flags: feedback?(mixedpuppy) → feedback+
(In reply to Shane Caraveo (:mixedpuppy) from comment #4)
> Aside from the feedback below, I want to understand the potential problems
> with allowing extensions to force discard.  Can you outline the behavior?

In a nut shell, calling tabs.discard(tabId, {forceDiscard: true}) is exactly the same as calling tabs.discard(tabId) with `dom.disable_beforeunload = true`

https://dxr.mozilla.org/mozilla-central/source/layout/base/nsDocumentViewer.cpp#1240-1242

No prompts will be shown (even if a beforeunload handler wants to) and all beforeunload handlers will run.
(In reply to Shane Caraveo (:mixedpuppy) from comment #4)

> >+const BASE_URL = "http://mochi.test:8888/browser/browser/components/extensions/test/browser/";
> 
> I know we do this everywhere, but I think you should be able to just use
> SimpleTest.getTestFileURL("filename.html");

This doesn't seem to work from browser/components/extensions/test/...

Returns:

chrome://browser/content/file_onbeforeunload.html

I need something equivalent to:

http://mochi.test:8888/browser/browser/components/extensions/test/browser/file_onbeforeunload.html

I don't find any examples of using this construct in browser/components/extensions/test/..., they all use what I used.

It seems to be for toolkit/components/extensions/test/mochitest
^^^^
Shane, do you automatically get notified of these?  Or am I supposed to be adding something in my commit message like r?= or something?
Flags: needinfo?(mixedpuppy)
Priority: -- → P3
Comment on attachment 8933112 [details]
Bug 1420681 - add forceDiscard property to tabs.discard - V4

https://reviewboard.mozilla.org/r/204088/#review210382

This all looks good.  The item causing me pause is chrome compat.

::: browser/components/extensions/schemas/tabs.json:898
(Diff revision 1)
>                {"type": "array", "items": {"type": "integer", "minimum": 0}}
>              ]
> +          },
> +          {
> +            "type": "object",
> +            "name": "discardProperties",

Adding this param makes the api incompatible with chrome.  Might not be a deal breaker, but want to highlight this and think about it.  Of course, having async: true also makes it incompatible.

::: browser/components/extensions/test/browser/browser_ext_tabs_discard.js:11
(Diff revision 1)
> +const BASE_URL = "http://mochi.test:8888/browser/browser/components/extensions/test/browser/";
> +
>  add_task(async function test_discarded() {
> +  await SpecialPowers.pushPrefEnv({
> +    "set": [
> +      ["dom.require_user_interaction_for_beforeunload", false],

default for this is true, why are testing with false?  Does this just disable the UI but not the effect otherwise?

::: browser/components/extensions/test/browser/browser_ext_tabs_discard.js:42
(Diff revision 1)
> +      browser.test.succeed("tab is discarded");
> +
> -        try {
> +      try {
> -          await browser.tabs.discard(tabs[0].id);
> +        await browser.tabs.discard(tabs[0].id);
> -          await browser.tabs.discard(tabs[2].id);
> +        await browser.tabs.discard(tabs[3].id);
> -          browser.test.succeed("attempting to discard an already discarded tab or the active tab should not throw error");
> +        browser.test.succeed("attempting to discard an already discarded tab or the active tab should not throw error");

nit: Can we do two seperate catches for these so if one fails, it will be very clear in the logs which it is?
Kevin, I'm wondering if we can do anything to handle chrome compat on this.
Assignee: nobody → kevinhowjones
Flags: needinfo?(mixedpuppy) → needinfo?(kevinhowjones)
(In reply to Shane Caraveo (:mixedpuppy) from comment #9)
> Comment on attachment 8933112 [details]
> Bug 1420681 - add forceDiscard property to tabs.discard
> 
> https://reviewboard.mozilla.org/r/204088/#review210382
> 
> This all looks good.  The item causing me pause is chrome compat.
> 
> ::: browser/components/extensions/schemas/tabs.json:898
> (Diff revision 1)
> >                {"type": "array", "items": {"type": "integer", "minimum": 0}}
> >              ]
> > +          },
> > +          {
> > +            "type": "object",
> > +            "name": "discardProperties",
> 
> Adding this param makes the api incompatible with chrome.  Might not be a
> deal breaker, but want to highlight this and think about it.

Well, I guess it would be incompatible for an extension written for Firefox to port to Chrome if this was used, but since the param is optional, it should be portable the other way around.  We already have lots of APIs which are not portable to Chrome.

> Of course, having async: true also makes it incompatible.

Agreed, I'll change that to a callback.

> ::: browser/components/extensions/test/browser/browser_ext_tabs_discard.js:11
> (Diff revision 1)
> > +const BASE_URL = "http://mochi.test:8888/browser/browser/components/extensions/test/browser/";
> > +
> >  add_task(async function test_discarded() {
> > +  await SpecialPowers.pushPrefEnv({
> > +    "set": [
> > +      ["dom.require_user_interaction_for_beforeunload", false],
> 
> default for this is true, why are testing with false?  Does this just
> disable the UI but not the effect otherwise?

This keeps us from having to fake a user interaction with the tab, otherwise there would be no attempt to show prompts.  Otherwise we are getting the same behavior as if the user had interacted.
 
> ::: browser/components/extensions/test/browser/browser_ext_tabs_discard.js:42
> (Diff revision 1)
> > +      browser.test.succeed("tab is discarded");
> > +
> > -        try {
> > +      try {
> > -          await browser.tabs.discard(tabs[0].id);
> > +        await browser.tabs.discard(tabs[0].id);
> > -          await browser.tabs.discard(tabs[2].id);
> > +        await browser.tabs.discard(tabs[3].id);
> > -          browser.test.succeed("attempting to discard an already discarded tab or the active tab should not throw error");
> > +        browser.test.succeed("attempting to discard an already discarded tab or the active tab should not throw error");
> 
> nit: Can we do two seperate catches for these so if one fails, it will be
> very clear in the logs which it is?

Indeed.
Flags: needinfo?(kevinhowjones)
Comment on attachment 8933112 [details]
Bug 1420681 - add forceDiscard property to tabs.discard - V4

https://reviewboard.mozilla.org/r/204088/#review210390

::: browser/components/extensions/test/browser/browser_ext_tabs_discard.js:26
(Diff revision 1)
> +      function awaitForDiscardTab(tabId, forceDiscard) {
> +        return new Promise(resolve => {
> +          browser.tabs.onUpdated.addListener(async function onUpdatedListener(updatedTabId, updatedInfo) {
> +            if ("discarded" in updatedInfo && updatedTabId == tabId) {
> +              browser.tabs.onUpdated.removeListener(onUpdatedListener);
> +              resolve(33);

Why 33?
(In reply to Kevin Jones from comment #11)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #9)

> > > +            "name": "discardProperties",
> > 
> > Adding this param makes the api incompatible with chrome.  Might not be a
> > deal breaker, but want to highlight this and think about it.
> 
> Well, I guess it would be incompatible for an extension written for Firefox
> to port to Chrome if this was used, but since the param is optional, it
> should be portable the other way around.  We already have lots of APIs which
> are not portable to Chrome.

I'm more concerned about a chrome extension ported to firefox.  Since we'd support a second param here, the chrome code would be passing a callback.  I guess schema would catch that, but if we can make it just work that would be preferable.  If we cant make it work, I would like a test to ensure that passing a callback as the second param will fail.

> > Of course, having async: true also makes it incompatible.
> 
> Agreed, I'll change that to a callback.

Only per my above comment on making the second param work, otherwise we can leave it as is.

> > ::: browser/components/extensions/test/browser/browser_ext_tabs_discard.js:11
> > (Diff revision 1)
> > > +const BASE_URL = "http://mochi.test:8888/browser/browser/components/extensions/test/browser/";
> > > +
> > >  add_task(async function test_discarded() {
> > > +  await SpecialPowers.pushPrefEnv({
> > > +    "set": [
> > > +      ["dom.require_user_interaction_for_beforeunload", false],
> > 
> > default for this is true, why are testing with false?  Does this just
> > disable the UI but not the effect otherwise?
> 
> This keeps us from having to fake a user interaction with the tab, otherwise
> there would be no attempt to show prompts.  Otherwise we are getting the
> same behavior as if the user had interacted.

Not understanding any of what you just said, so I went to the code.  Basically setting this to false seems to allow firefox to present the dialog even if the user never interacted with the tab, so setting it to false helps with testing.
(In reply to Shane Caraveo (:mixedpuppy) from comment #13)
> (In reply to Kevin Jones from comment #11)
> > (In reply to Shane Caraveo (:mixedpuppy) from comment #9)
> 
> > > > +            "name": "discardProperties",
> > > 
> > > Adding this param makes the api incompatible with chrome.  Might not be a
> > > deal breaker, but want to highlight this and think about it.
> > 
> > Well, I guess it would be incompatible for an extension written for Firefox
> > to port to Chrome if this was used, but since the param is optional, it
> > should be portable the other way around.  We already have lots of APIs which
> > are not portable to Chrome.
> 
> I'm more concerned about a chrome extension ported to firefox.  Since we'd
> support a second param here, the chrome code would be passing a callback.  I
> guess schema would catch that, but if we can make it just work that would be
> preferable.  If we cant make it work, I would like a test to ensure that
> passing a callback as the second param will fail.

Oh yes, of course :-).  It would seem like we should be able to make it work since the schema allows different options for parameter types.
 
> > > Of course, having async: true also makes it incompatible.
> > 
> > Agreed, I'll change that to a callback.
> 
> Only per my above comment on making the second param work, otherwise we can
> leave it as is.
> 
> > > ::: browser/components/extensions/test/browser/browser_ext_tabs_discard.js:11
> > > (Diff revision 1)
> > > > +const BASE_URL = "http://mochi.test:8888/browser/browser/components/extensions/test/browser/";
> > > > +
> > > >  add_task(async function test_discarded() {
> > > > +  await SpecialPowers.pushPrefEnv({
> > > > +    "set": [
> > > > +      ["dom.require_user_interaction_for_beforeunload", false],
> > > 
> > > default for this is true, why are testing with false?  Does this just
> > > disable the UI but not the effect otherwise?
> > 
> > This keeps us from having to fake a user interaction with the tab, otherwise
> > there would be no attempt to show prompts.  Otherwise we are getting the
> > same behavior as if the user had interacted.
> 
> Not understanding any of what you just said, so I went to the code. 
> Basically setting this to false seems to allow firefox to present the dialog
> even if the user never interacted with the tab, so setting it to false helps
> with testing.

Yes, exactly.
(In reply to Shane Caraveo (:mixedpuppy) from comment #12)
> Comment on attachment 8933112 [details]
> Bug 1420681 - add forceDiscard property to tabs.discard
> 
> https://reviewboard.mozilla.org/r/204088/#review210390
> 
> ::: browser/components/extensions/test/browser/browser_ext_tabs_discard.js:26
> (Diff revision 1)
> > +      function awaitForDiscardTab(tabId, forceDiscard) {
> > +        return new Promise(resolve => {
> > +          browser.tabs.onUpdated.addListener(async function onUpdatedListener(updatedTabId, updatedInfo) {
> > +            if ("discarded" in updatedInfo && updatedTabId == tabId) {
> > +              browser.tabs.onUpdated.removeListener(onUpdatedListener);
> > +              resolve(33);
> 
> Why 33?

Diagnostic artifact.  I'll remove it.
(In reply to Kevin Jones from comment #16)
> Comment on attachment 8933112 [details]
> Bug 1420681 - add forceDiscard property to tabs.discard - V2
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/204088/diff/1-2/

Shane, what do you think?
Comment on attachment 8933112 [details]
Bug 1420681 - add forceDiscard property to tabs.discard - V4

https://reviewboard.mozilla.org/r/204088/#review210738

::: browser/components/extensions/ext-tabs.js:466
(Diff revisions 1 - 2)
>                                                      discardProperties.forceDiscard);
>            }
> +
> +          // For Chrome compatibility, where only a single tab can be discarded at a time.
> +          // If tab was not discarded, return undefined.
> +          return tabs[0].linkedPanel ? undefined : tabManager.convert(tabs[0]);

Should we only return something if the call was chrome compatible?  ie. discard(tabid) returns a result but discard([...tabIds]) does not.  It seems weird to return a single tab if a list of tabs was requested.  Another option could be to return the list if discarded tabs.  What do you think?

::: browser/components/extensions/schemas/tabs.json:910
(Diff revisions 1 - 2)
>                }
>              }
> +          },
> +          {
> +            "type": "function",
> +            "name": "callback",

It seems to me that discardProperties and callback should be mutually exclusive within a choice.  That way we support chrome api:

chrome.tabs.discard(tab, result => {})

OR

result = await browser.tabs.discard(tab, {forceDiscard: true})

but not a combination of forceDiscard and callback.

OTOH I'm not certain if that is possible (ie. it may not work in schema)

::: browser/components/extensions/test/browser/browser_ext_tabs_discard.js:73
(Diff revisions 1 - 2)
> +      await new Promise(resolve => {
> +        let callback = (tab) => {
> +          browser.test.assertEq(undefined, tab, "callback runs when attempting to discard a tab which cannot be discarded, and returns undefined");
> +          resolve();
> +        };
> +        browser.tabs.discard(tabs[3].id, callback);

Depending on whether my schema suggestion works...

it works:

add a test that you cannot use both forceDiscard and callback.

it doesn't:

add a test that verifies a call with both force and callback works.
Kevin, comment 18 is entirely discussion/questioning on how it should work, lets decide before making changes.
(In reply to Shane Caraveo (:mixedpuppy) from comment #18)
> Comment on attachment 8933112 [details]
> Bug 1420681 - add forceDiscard property to tabs.discard - V2
> 
> https://reviewboard.mozilla.org/r/204088/#review210738
> 
> ::: browser/components/extensions/ext-tabs.js:466
> (Diff revisions 1 - 2)
> >                                                      discardProperties.forceDiscard);
> >            }
> > +
> > +          // For Chrome compatibility, where only a single tab can be discarded at a time.
> > +          // If tab was not discarded, return undefined.
> > +          return tabs[0].linkedPanel ? undefined : tabManager.convert(tabs[0]);
> 
> Should we only return something if the call was chrome compatible?  ie.
> discard(tabid) returns a result but discard([...tabIds]) does not.  It seems
> weird to return a single tab if a list of tabs was requested.  Another
> option could be to return the list if discarded tabs.  What do you think?

If we return a list, doesn't that break Chrome compatibility?  But I can see testing tabs.length and not returning anything if there is more than one member.

> ::: browser/components/extensions/schemas/tabs.json:910
> (Diff revisions 1 - 2)
> >                }
> >              }
> > +          },
> > +          {
> > +            "type": "function",
> > +            "name": "callback",
> 
> It seems to me that discardProperties and callback should be mutually
> exclusive within a choice.  That way we support chrome api:
> 
> chrome.tabs.discard(tab, result => {})
> 
> OR
> 
> result = await browser.tabs.discard(tab, {forceDiscard: true})
> 
> but not a combination of forceDiscard and callback.
> 
> OTOH I'm not certain if that is possible (ie. it may not work in schema)

Hmmm, nothing is coming to me offhand as a way to handle that in the schema, all I can think of is handling it in the API (reject or whatever if 3 args are given)

But does it really matter?  I don't see how not having them mutually exclusive breaks Chrome compat.
(In reply to Kevin Jones from comment #20)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #18)
> > Comment on attachment 8933112 [details]
> > Bug 1420681 - add forceDiscard property to tabs.discard - V2
> > 
> > https://reviewboard.mozilla.org/r/204088/#review210738
> > 
> > ::: browser/components/extensions/ext-tabs.js:466
> > (Diff revisions 1 - 2)
> > >                                                      discardProperties.forceDiscard);
> > >            }
> > > +
> > > +          // For Chrome compatibility, where only a single tab can be discarded at a time.
> > > +          // If tab was not discarded, return undefined.
> > > +          return tabs[0].linkedPanel ? undefined : tabManager.convert(tabs[0]);
> > 
> > Should we only return something if the call was chrome compatible?  ie.
> > discard(tabid) returns a result but discard([...tabIds]) does not.  It seems
> > weird to return a single tab if a list of tabs was requested.  Another
> > option could be to return the list if discarded tabs.  What do you think?

chromeCompat = callbackProvided and recieved singleTabId
if chromeCompat return oneID
else return listOfIDs

> If we return a list, doesn't that break Chrome compatibility?  But I can see
> testing tabs.length and not returning anything if there is more than one
> member.
> 
> > ::: browser/components/extensions/schemas/tabs.json:910
> > (Diff revisions 1 - 2)
> > >                }
> > >              }
> > > +          },
> > > +          {
> > > +            "type": "function",
> > > +            "name": "callback",
> > 
> > It seems to me that discardProperties and callback should be mutually
> > exclusive within a choice.  That way we support chrome api:
> > 
> > chrome.tabs.discard(tab, result => {})
> > 
> > OR
> > 
> > result = await browser.tabs.discard(tab, {forceDiscard: true})
> > 
> > but not a combination of forceDiscard and callback.
> > 
> > OTOH I'm not certain if that is possible (ie. it may not work in schema)
> 
> Hmmm, nothing is coming to me offhand as a way to handle that in the schema,
> all I can think of is handling it in the API (reject or whatever if 3 args
> are given)

choice: [detailsObject, callback] could do it, what I don't know is if defining a callback that way will work with the schema parsing.

> But does it really matter?  I don't see how not having them mutually
> exclusive breaks Chrome compat.

It doesn't break compat.  Think of it as two modes, one is chrome compat in which case it can take only one id and a callback.  Non-compat takes a list and options.  

I'm not certain this matters.
(In reply to Shane Caraveo (:mixedpuppy) from comment #21)
> chromeCompat = callbackProvided and recieved singleTabId
> if chromeCompat return oneID
> else return listOfIDs

nit: according to the chrome spec, we should return a tabs.Tab object rather than an ID.  I guess for non-chrome we could return either if we want.

> > Hmmm, nothing is coming to me offhand as a way to handle that in the schema,
> > all I can think of is handling it in the API (reject or whatever if 3 args
> > are given)
> 
> choice: [detailsObject, callback] could do it, what I don't know is if
> defining a callback that way will work with the schema parsing.

Maybe we can chat some time about this, I don't really see what you are seeing.

> > But does it really matter?  I don't see how not having them mutually
> > exclusive breaks Chrome compat.
> 
> It doesn't break compat.  Think of it as two modes, one is chrome compat in
> which case it can take only one id and a callback.  Non-compat takes a list
> and options.  
> 
> I'm not certain this matters.

I suppose if there is a simple way to do this in the schema we could go ahead and do it, otherwise, just not worry about it.
(In reply to Kevin Jones from comment #22)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #21)
> > chromeCompat = callbackProvided and recieved singleTabId
> > if chromeCompat return oneID
> > else return listOfIDs
> 
> nit: according to the chrome spec, we should return a tabs.Tab object rather
> than an ID.  I guess for non-chrome we could return either if we want.

it was merely an illustration.

> > > Hmmm, nothing is coming to me offhand as a way to handle that in the schema,
> > > all I can think of is handling it in the API (reject or whatever if 3 args
> > > are given)
> > 
> > choice: [detailsObject, callback] could do it, what I don't know is if
> > defining a callback that way will work with the schema parsing.
> 
> Maybe we can chat some time about this, I don't really see what you are
> seeing.

search for "choices" in the schema directories.

> > > But does it really matter?  I don't see how not having them mutually
> > > exclusive breaks Chrome compat.
> > 
> > It doesn't break compat.  Think of it as two modes, one is chrome compat in
> > which case it can take only one id and a callback.  Non-compat takes a list
> > and options.  
> > 
> > I'm not certain this matters.
> 
> I suppose if there is a simple way to do this in the schema we could go
> ahead and do it, otherwise, just not worry about it.

+1
(In reply to Shane Caraveo (:mixedpuppy) from comment #23)
> search for "choices" in the schema directories.

Oh yes, I see what you are saying now.  I actually tried that approach when experimenting this morning but got "incorrect parameter types" error, but maybe I don't quite have it constructed correctly.  Here is the code that I tried (albeit only the `discardProperties` choice):

https://pastebin.com/91MAr309
I've probably gone down a rabit hole.  If all these work, it's probably fine the way you have it (aside from missing tests)

discard(id, callback)
discard(id|ids, options)
discard(id|ids, options, callback)
(In reply to Kevin Jones from comment #27)
> Comment on attachment 8933112 [details]
> Bug 1420681 - add forceDiscard property to tabs.discard - V3
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/204088/diff/2-3/

There is a bug in the tests which I haven't been able to nail down:

42 INFO TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/test-oop-extensions/browser_ext_tabs_discard.js | uncaught exception - TypeError: browser.frameLoader is null at setTabState@chrome://browser/content/tabbrowser.xml:4421:19
unloadNonRequiredTabs@chrome://browser/content/tabbrowser.xml:4840:19
onUnloadTimeout@chrome://browser/content/tabbrowser.xml:4816:15
suppressDisplayPortAndQueueUnload/this.unloadTimer<@chrome://browser/content/tabbrowser.xml:5110:54

Apparently race condition of some kind, as I am able to make it disappear using time delays.

But I thought I would push the changes I have for examination.
I will no longer be able to spend time contributing to bugs for Mozilla.  I am un-assigning myself from this bug.
Assignee: kevinhowjones → nobody
Hi,

I would like to automatically discard some tabs. But, it is silently failing because of the event beforeunload set on some tabs. (Basic example: https://github.com/Morikko/sync-tab-groups/issues).

The same extension on Chrome is discarding without objection.

Any progression for the feature of this issue ?

Also, a feedback when a tab is not discarded in case of beforeunload event would be appreciated. (Maybe in the returned Promise...).

Cheers,
Product: Toolkit → WebExtensions
Bulk move of bugs per https://bugzilla.mozilla.org/show_bug.cgi?id=1483958
Component: Untriaged → General
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: