Closed Bug 1575891 Opened 5 years ago Closed 5 years ago

NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIURI.hostPort]

Categories

(WebExtensions :: General, defect, P3)

70 Branch
x86_64
Windows 10
defect

Tracking

(firefox72 fixed)

RESOLVED FIXED
mozilla72
Tracking Status
firefox72 --- fixed

People

(Reporter: vtol, Assigned: rpl)

References

Details

Attachments

(4 files)

  • FX 70 x_64 build dcfcd7909aff0ef81a3b884ead0745645c6d6670
  • W10 Pro x_64 1903 b18362.295

browser console repeatedly prints

NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIURI.hostPort] ext-browsingData.js:192

pointing to

let host = principal.URI.hostPort;

No idea what triggers it

Priority: -- → P3

Can you please let us know if you have any addons installed and if yes, which ones?

Flags: needinfo?(vtol)
Attached file wx info (deleted) —
Flags: needinfo?(vtol)
Attached image recent log (deleted) —

I installed all extensions described above on FX 70.0b4, Build ID 20190905174512, Win10x64 and I can confirm that the errors described are displayed in console. It can be simply reproduced by opening Spotify login page .

I will attach a screenshot with my browser console.

Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached image NS_ERROR.png (deleted) —

This is happening in clearLocalStorage.

Blocks: 1286798
Flags: needinfo?(lgreco)

The error output shown in comment 5 seems to be different, and might even be unrelated, than what been previously/initially reported

NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIURI.hostPort] ext-browsingData.js:194

This probably wants a change like the fix from https://bugzilla.mozilla.org/show_bug.cgi?id=1507171#c13 to only try and inspect the host/hostPort of URI types that actually have hosts. It's likely an "about:newtab" principal or similar is now using storage in the profile.

Of course, in general, the recommendation continues to be to use Sanitizer.jsm and its "offlineApps" category. (But which does constitute a change in API behavior.)

Andrew is right, the issue is definitely triggered when an about page is storing data.

"Ironically" the about pages don't seem to actually support localStorage at all, accessing localStorage from an about page throws an Exception "Component is not available" nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE) error.

Nevertheless, we do try to clear localStorage for any principal that has stored any kind of data when Services.lsm.nextGenLocalStorageEnabled is true (and it looks that we enabled it on all channels starting from Firefox 68, Bug 1539835):

https://searchfox.org/mozilla-central/rev/5cb522c7baba24e55874809e0e206b001494c1e9/browser/components/extensions/parent/ext-browsingData.js#175-194

and so the fact that about:newtab (or any other about page) stores some data into IndexedDB is going to trigger this issue.

I do agree with Andrew that we should definitely look into fixing Bug 1531276 and make the browserData API to reuse as much as possible Sanitizer.jsm/nsIClearDataService, to avoid keeping fixing this kind of issues twice (once for Firefox sanitizer and once more for the WebExtensions APIs).

In the meantime I've attached a patch with a small change to fix the regression and an additional test case (given that this has regressed on Firefox 68 and this smaller fix is a low risky change, in case we may want to uplift a fix for this regression on ESR).

Flags: needinfo?(lgreco)
Attachment #9099549 - Attachment description: Bug 1575891 - Fix browserData.remove regression on nextGen localStorage and indexedDB data stored by an about page. r?mixedpuppy!,asuth → Bug 1575891 - browsingData API should not clear nextGen localStorage data stored by browser components and extensions. r?mixedpuppy!,asuth,johannh
Version: Firefox 70 → 70 Branch
Pushed by luca.greco@alcacoop.it: https://hg.mozilla.org/integration/autoland/rev/4b4c2fa6e6fa browsingData API should not clear nextGen localStorage data stored by browser components and extensions. r=mixedpuppy,asuth,johannh
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla72
Assignee: nobody → lgreco

Comment on attachment 9099549 [details]
Bug 1575891 - browsingData API should not clear nextGen localStorage data stored by browser components and extensions. r?mixedpuppy!,asuth,johannh

Beta/Release Uplift Approval Request

  • User impact if declined: If the nextGenLocalStorage is going to be turned back on in Firefox 71, then for any user that used extensions based on the browsingData WebExtensions API:
  • clearing the localStorage data is not going to work anymore, because the API will throw while trying to access hostPort on a URI with the about scheme (if any Firefox integrated about:* page has any data stored in a WebAPI storage, it doesn't need to be stored in localStorage),

  • or the API may actually clear also any localStorage data stored by all extensions (because before this fix the API is enumerating all the origins with some data stored and then it tries to clear the nextGenLocalStorage data from the origin without checking if the schema is one of those only related to web pages, e.g. it would also clear moz-extension origins which are actually related to other extensions).

  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): The change is pretty simple and limited to the part of the browsingData API implementation that clean the data stored into the nextGenLocalStorage. The patch also include two new test cases (which is basically half of the small patch) to explicitly test the fix as part of the automated tests (and prevent it from regressing).
  • String changes made/needed:
Attachment #9099549 - Flags: approval-mozilla-beta?
Flags: qe-verify-

Comment on attachment 9099549 [details]
Bug 1575891 - browsingData API should not clear nextGen localStorage data stored by browser components and extensions. r?mixedpuppy!,asuth,johannh

Uplift approved for 71 beta 10, thanks.

Attachment #9099549 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Backed out changeset b6846fa525a2 (Bug 1575891) for causing browser-chrome failures in browser_ext_browsingData_localStorage.js

Backout link: https://hg.mozilla.org/releases/mozilla-beta/rev/992520c31cde6750358c5d5c3a610f77ebadd404

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&selectedJob=276208517&resultStatus=testfailed%2Cbusted%2Cexception&tochange=992520c31cde6750358c5d5c3a610f77ebadd404&fromchange=b6846fa525a2dd976e16339215c3d63c8fe67245

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=276208517&repo=mozilla-beta&lineNumber=5075

[task 2019-11-14T15:39:57.631Z] 15:39:57 INFO - Console message: Warning: attempting to write 6412 bytes to preference extensions.webextensions.uuids. This is bad for general performance and memory usage. Such an amount of data should rather be written to an external file. This preference will not be sent to any content processes.
[task 2019-11-14T15:39:57.632Z] 15:39:57 INFO - Buffered messages finished
[task 2019-11-14T15:39:57.633Z] 15:39:57 INFO - TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/browser_ext_browsingData_localStorage.js | Got the expected localStorage data - Got null, expected value
[task 2019-11-14T15:39:57.633Z] 15:39:57 INFO - Stack trace:
[task 2019-11-14T15:39:57.633Z] 15:39:57 INFO - chrome://mochikit/content/browser-test.js:test_is:1312
[task 2019-11-14T15:39:57.634Z] 15:39:57 INFO - chrome://mochitests/content/browser/browser/components/extensions/test/browser/browser_ext_browsingData_localStorage.js:test_browserData_should_not_remove_extension_data:152
[task 2019-11-14T15:39:57.635Z] 15:39:57 INFO - Leaving test bound test_browserData_should_not_remove_extension_data
[task 2019-11-14T15:39:57.637Z] 15:39:57 INFO - GECKO(1152) | MEMORY STAT | vsize 3066MB | residentFast 445MB | heapAllocated 187MB

Flags: needinfo?(lgreco)

I discussed about this with Aryx over IRC, and it is fine to revert this patch if we have also disabled LSNG in Firefox 71 Beta.

The failure is triggered by the second test case added in this patch: 'test_browserData_should_not_remove_extension_data',
which pass successfully when LSNG is enabled, but fails when it is disabled.

When LSNG is disabled, the browsingData API does clear the localStorage data by notify a "extension:purge-localStorage" message over the observer service, when the browserData API call doesn't specify a list of specific origins to clear the code that handles "extension:purge-localStorage" will clear all the localStorage data, including the one stored by extensions:

I filed Bug 1596542 to make sure that this particular test case is going to be skipped if LSNG has to be disabled on Firefox 72 (which should be unlikely, but it would still be better to prevent upfront to make it more complicate in case we have to do it during the beta cycle or in a dot release).

Flags: needinfo?(lgreco)

Can you remove the approval-mozilla-beta+ to prevent this bug from appearing in the uplift queries, please?

Flags: needinfo?(pascalc)
Flags: needinfo?(pascalc)
Attachment #9099549 - Flags: approval-mozilla-beta+ → approval-mozilla-beta-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: