Closed Bug 1364852 Opened 7 years ago Closed 7 years ago

JavaScript error: chrome://browser/content/preferences/siteDataSettings.js, line 225: NS_ERROR_INSUFFICIENT_DOMAIN_LEVELS: Component returned failure code: 0x804b0050 (NS_ERROR_INSUFFICIENT_DOMAIN_LEVELS) [nsIEffectiveTLDService.getBaseDomainFromHost]

Categories

(Firefox :: Settings UI, defect)

Product:
Firefox
Component:
Settings UI
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: shawnjohnjr, Assigned: Fischer)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Bug 1364852 - Handle error from Services.eTLD.getBaseDomainFromHost,
59 bytes, text/x-review-board-request
Gijs
: review+
Details 
STR:
1. Enter Security&Privacy
2. Enter Site data (Settings)
3. Remove one origin data
4. Remove operation fail

Error:

JavaScript error: chrome://browser/content/preferences/siteDataSettings.js, line 225: NS_ERROR_INSUFFICIENT_DOMAIN_LEVELS: Component returned failure code: 0x804b0050 (NS_ERROR_INSUFFICIENT_DOMAIN_LEVELS) [nsIEffectiveTLDService.getBaseDomainFromHost]
Assignee: nobody → shuang
The site I have seen with error is s3-us-west-2.amazonaws.com.
Comment on attachment 8867650 [details] [diff] [review]
Bug 1364852 - Handle NS_ERROR_INSUFFICIENT_DOMAIN_LEVELS for getBaseDomainFromHost

Review of attachment 8867650 [details] [diff] [review]:
-----------------------------------------------------------------

Stealing because this popped up in my bugmail and I have comments. :-)

How are we grouping the hosts otherwise? Are we not using the nsIEffectiveTLDService in some place where we *should* be using it? Otherwise, how come we end up with hosts that lack subdomains (ie are effectively acting as tlds, like the amazonaws S3 hosts) ?

::: browser/components/preferences/siteDataSettings.js
@@ +216,5 @@
> +            baseDomain = Services.eTLD.getBaseDomainFromHost(host);
> +          } catch (e) {
> +            if (e.result == Cr.NS_ERROR_HOST_IS_IP_ADDRESS ||
> +                e.result == Cr.NS_ERROR_INSUFFICIENT_DOMAIN_LEVELS) {
> +              baseDomain = host;

We should re-throw the error in the else case, otherwise `baseDomain` will be undefined if there's some other kind of error, and we'll assign to the `undefined` key in the hosts table and all kinds of bad things might happen then.

Also, this should live in a helper method so we don't need to repeat it.
Attachment #8867650 - Flags: review?(fliu) → review-
(In reply to :Gijs from comment #3)
> Comment on attachment 8867650 [details] [diff] [review]
> Bug 1364852 - Handle NS_ERROR_INSUFFICIENT_DOMAIN_LEVELS for
> getBaseDomainFromHost
> 
> Review of attachment 8867650 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Stealing because this popped up in my bugmail and I have comments. :-)
Thanks for reviewing.
> 
> How are we grouping the hosts otherwise? Are we not using the
> nsIEffectiveTLDService in some place where we *should* be using it?
> Otherwise, how come we end up with hosts that lack subdomains (ie are
> effectively acting as tlds, like the amazonaws S3 hosts) ?

You're right. I did not dig into the problem enough.
I think |nsEffectiveTLDService::GetBaseDomainInternal| doesn't give the correct results.
So we should fix that first.

> 
> ::: browser/components/preferences/siteDataSettings.js
> @@ +216,5 @@
> > +            baseDomain = Services.eTLD.getBaseDomainFromHost(host);
> > +          } catch (e) {
> > +            if (e.result == Cr.NS_ERROR_HOST_IS_IP_ADDRESS ||
> > +                e.result == Cr.NS_ERROR_INSUFFICIENT_DOMAIN_LEVELS) {
> > +              baseDomain = host;
> 
> We should re-throw the error in the else case, otherwise `baseDomain` will
> be undefined if there's some other kind of error, and we'll assign to the
> `undefined` key in the hosts table and all kinds of bad things might happen
> then.
> 
> Also, this should live in a helper method so we don't need to repeat it.

Sure.
(In reply to Shawn Huang [:shawnjohnjr] from comment #4)
> (In reply to :Gijs from comment #3)
> > Comment on attachment 8867650 [details] [diff] [review]
> > Bug 1364852 - Handle NS_ERROR_INSUFFICIENT_DOMAIN_LEVELS for
> > getBaseDomainFromHost
> > 
> > Review of attachment 8867650 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Stealing because this popped up in my bugmail and I have comments. :-)
> Thanks for reviewing.
> > 
> > How are we grouping the hosts otherwise? Are we not using the
> > nsIEffectiveTLDService in some place where we *should* be using it?
> > Otherwise, how come we end up with hosts that lack subdomains (ie are
> > effectively acting as tlds, like the amazonaws S3 hosts) ?
> 
> You're right. I did not dig into the problem enough.
> I think |nsEffectiveTLDService::GetBaseDomainInternal| doesn't give the
> correct results.
> So we should fix that first.
> 
> > 
> > ::: browser/components/preferences/siteDataSettings.js
> > @@ +216,5 @@
> > > +            baseDomain = Services.eTLD.getBaseDomainFromHost(host);
> > > +          } catch (e) {
> > > +            if (e.result == Cr.NS_ERROR_HOST_IS_IP_ADDRESS ||
> > > +                e.result == Cr.NS_ERROR_INSUFFICIENT_DOMAIN_LEVELS) {
> > > +              baseDomain = host;
> > 
> > We should re-throw the error in the else case, otherwise `baseDomain` will
> > be undefined if there's some other kind of error, and we'll assign to the
> > `undefined` key in the hosts table and all kinds of bad things might happen
> > then.
> > 
> > Also, this should live in a helper method so we don't need to repeat it.
> 
> Sure.

https://bugzilla.mozilla.org/show_bug.cgi?id=368989#c26
https://bugzilla.mozilla.org/show_bug.cgi?id=368989#c29
Fischer, do you plan to change api to get base domain? Do I need to transfer this bug to you?
Flags: needinfo?(fliu)
To clarify that it's not because 'facebook.com', it's because the code scan sites and try to extract base domain.
However, one site "s3-us-west-2.amazonaws.com" in that list, and it's defined in etld_data.inc [1].

And getBaseDomainFromHost gets eTLD+1, but site "s3-us-west-2.amazonaws.com" is already in the list. So it throws NS_ERROR_INSUFFICIENT_DOMAIN_LEVELS. This exception is thrown if there were insufficient subdomain levels in the hostname to satisfy the requested aAdditionalParts value. So it expects at least one part.

[1] https://dxr.mozilla.org/mozilla-central/source/obj-x86_64-pc-linux-gnu/netwerk/dns/etld_data.inc#4709
Assignee: shuang → fliu
Comment on attachment 8867650 [details] [diff] [review]
Bug 1364852 - Handle NS_ERROR_INSUFFICIENT_DOMAIN_LEVELS for getBaseDomainFromHost

Thanks Shawn's help on this bug. Another new patch is pushed so make this one as obsolete to avoid confusing.
Attachment #8867650 - Attachment is obsolete: true
Comment on attachment 8868644 [details]
Bug 1364852 - Handle error from Services.eTLD.getBaseDomainFromHost,

https://reviewboard.mozilla.org/r/140224/#review143556

::: browser/components/preferences/siteDataSettings.js:189
(Diff revision 1)
> +      // - NS_ERROR_INSUFFICIENT_DOMAIN_LEVELS: 2 cases for this error.
> +      //   The 1st one: the host is empty, which is always not possible
> +      //   since our host is from the gecko internal.
> +      //   The 2nd one: the host is already an eTLD one, say "s3-us-west-2.amazonaws.com",
> +      //   so no more part could be extracted from.
> +      //   For this case, just return the host as the result.

This is still confusing. Why are we hitting this case at all? Do we show the s3 domain in the list? Why? If it's an eTLD it shouldn't be possible to have shared storage across the entire eTLD - that's like sharing indexeddb or other storage across all of '.com'. So I would expect this to not be returned by the quota or cache APIs. Has someone actually checked how this origin ended up being listed in the first place?
Attachment #8868644 - Flags: review?(gijskruitbosch+bugs)
(In reply to :Gijs from comment #10)
> Comment on attachment 8868644 [details]
> Bug 1364852 - Handle error from Services.eTLD.getBaseDomainFromHost,
> 
> https://reviewboard.mozilla.org/r/140224/#review143556
> 
> ::: browser/components/preferences/siteDataSettings.js:189
> (Diff revision 1)
> > +      // - NS_ERROR_INSUFFICIENT_DOMAIN_LEVELS: 2 cases for this error.
> > +      //   The 1st one: the host is empty, which is always not possible
> > +      //   since our host is from the gecko internal.
> > +      //   The 2nd one: the host is already an eTLD one, say "s3-us-west-2.amazonaws.com",
> > +      //   so no more part could be extracted from.
> > +      //   For this case, just return the host as the result.
> 
> This is still confusing. Why are we hitting this case at all? Do we show the
> s3 domain in the list? Why? If it's an eTLD it shouldn't be possible to have
> shared storage across the entire eTLD - that's like sharing indexeddb or
> other storage across all of '.com'. So I would expect this to not be
> returned by the quota or cache APIs. Has someone actually checked how this
> origin ended up being listed in the first place?

Well, I don't know. I used my daily use profile to test.
s3-us-west-2.amazonaws.com is in the storage/default folder. 
And under "http+++s3-us-west-2.amazonaws.com" folder, there is one indexeddb database.
So I guess S3 site used to call indexeddb api and storage something before.

$ ~/code/mozilla-inbound/obj-x86_64-pc-linux-gnu/tmp/scratch_user/storage/default$ ls -l
total 236
drwxr-xr-x 3 shawnjohnjr shawnjohnjr 4096 Dec 19 15:12 http+++inte.sogou.com
drwxr-xr-x 3 shawnjohnjr shawnjohnjr 4096 Jan 28 21:38 http+++joey1972se.pixnet.net
drwxr-xr-x 3 shawnjohnjr shawnjohnjr 4096 Mar 31 17:02 http+++masaejapan.pixnet.net
drwxr-xr-x 3 shawnjohnjr shawnjohnjr 4096 Apr  3 19:05 http+++minlabear14.pixnet.net
drwxr-xr-x 3 shawnjohnjr shawnjohnjr 4096 Jan 28 21:35 http+++mishel.pixnet.net
drwxr-xr-x 3 shawnjohnjr shawnjohnjr 4096 Jan 19 13:00 http+++portfoliobuildingday.com
drwxr-xr-x 3 shawnjohnjr shawnjohnjr 4096 Dec 19 15:12 http+++s3-us-west-2.amazonaws.com
drwxr-xr-x 4 shawnjohnjr shawnjohnjr 4096 Dec 19 15:12 https+++4fun.tw
drwxr-xr-x 3 shawnjohnjr shawnjohnjr 4096 Dec 19 15:12 https+++app.kktv.me
drwxr-xr-x 3 shawnjohnjr shawnjohnjr 4096 Dec 19 15:12 https+++drive.google.com

We suppose to get eTLD+1 domain storage not eTLD but in the specification:

https://storage.spec.whatwg.org/#terminology
A schemeless origin group
is a group of one of the following:

- Identical opaque origins.
- Tuple origins whose host is identical and not a domain.
- Tuple origins whose host is a domain of which the registrable domain is identical.
(In reply to Shawn Huang [:shawnjohnjr] from comment #11)
> (In reply to :Gijs from comment #10)
> > This is still confusing. Why are we hitting this case at all?
> 
> Well, I don't know. I used my daily use profile to test.
> s3-us-west-2.amazonaws.com is in the storage/default folder. 
> And under "http+++s3-us-west-2.amazonaws.com" folder, there is one indexeddb
> database.
> So I guess S3 site used to call indexeddb api and storage something before.

OK. It might be possible to check what is in the indexeddb and when that data was last touched, to see if it simply predates when these hosts were added to the public suffix lists.

I will file a bug for us to be more deliberate about changes to the PSL. We should probably be removing cookies, storage, permissions, and other items, and we should definitely ensure that if they exist, they do not get sent and aren't accessible once the PSL changes.

In the meantime, for this bug: I think the patch is close but the idea behind the previous patch, of filtering out only this particular exception, was better, IMO. We shouldn't make assumptions in this code about how the eTLD service behaves. We should specifically filter the "not enough domain parts" error, and swallow that and use the host, but for everything else we should rethrow the original error, because as the comment in the current version of the patch notes, we don't *expect* there to be errors, and if something is seriously wrong then we should learn about this and not ignore it. The set of errors that the eTLD service throws could change from under us, and then the comment will be out of date and we will just be swallowing exceptions that maybe we should be handling differently.
Depends on: 1365892
(In reply to :Gijs from comment #12)
> In the meantime, for this bug: I think the patch is close but the idea
> behind the previous patch, of filtering out only this particular exception,
> was better, IMO. We shouldn't make assumptions in this code about how the
> eTLD service behaves. We should specifically filter the "not enough domain
> parts" error, and swallow that and use the host, but for everything else we
> should rethrow the original error, because as the comment in the current
> version of the patch notes, we don't *expect* there to be errors, and if
> something is seriously wrong then we should learn about this and not ignore
> it. The set of errors that the eTLD service throws could change from under
> us, and then the comment will be out of date and we will just be swallowing
> exceptions that maybe we should be handling differently.
Hi Gijs,
Updated the patch.
Other unexpected error hosts are reported and excluded from removals so they would get noticed.
Thanks.
Comment on attachment 8868644 [details]
Bug 1364852 - Handle error from Services.eTLD.getBaseDomainFromHost,

https://reviewboard.mozilla.org/r/140224/#review145218

::: commit-message-6e3ca:3
(Diff revision 2)
> +Bug 1364852 - Handle error from Services.eTLD.getBaseDomainFromHost, r?Gijs
> +
> +This patch handles error while extracting base domain from hosts returned from Quota Manager or appcache and updates test cases accordingly.

Instead, describe *why* we're making this change, that is, why these errors happen, and link to the bug that is supposed to deal with this in a more thorough way.

::: browser/components/preferences/siteDataSettings.js:179
(Diff revision 2)
>      }
>      this._updateButtonsState();
>    },
>  
> +  _getBaseDomainFromHost(host) {
> +    let result;

We don't want to return uninitialized, so please initialize to host.

::: browser/components/preferences/siteDataSettings.js:190
(Diff revision 2)
> +        // For this 2 expected errors, just take the host as the result.
> +        // - NS_ERROR_HOST_IS_IP_ADDRESS: the host is in ipv4/ipv6.
> +        // - NS_ERROR_INSUFFICIENT_DOMAIN_LEVELS: not enough domain part to extract.
> +        result = host;
> +      } else {
> +        Cu.reportError(e);

Please just throw e;

::: browser/components/preferences/siteDataSettings.js:192
(Diff revision 2)
> +    } finally {
> +      return result;

Please remove the `finally` block and just return at the end of the function.

::: browser/components/preferences/siteDataSettings.js:233
(Diff revision 2)
>          // We will remove cookies based on base domain, say, user selects "news.foo.com" to remove.
>          // The cookies under "music.foo.com" will be removed together.
>          // We have to prompt user about this action.
>          let hostsTable = new Map();
>          // Group removed sites by base domain
> -        for (let host of removals) {
> +        for (let host of removals.values()) {

You can iterate over a set directly, ie you don't need the `.values()` bit.

::: browser/components/preferences/siteDataSettings.js:235
(Diff revision 2)
> +          if (!baseDomain) {
> +            // Something wrong while getting base domain.
> +            // Delete this entry from the target removals.
> +            removals.delete(host);
> +            continue;
> +          }

We should never hit this case, so we can just not catch errors here.

In any case, removing items from the set while iterating over it likely doesn't work correctly anyway...

::: browser/components/preferences/siteDataSettings.js:249
(Diff revision 2)
> +        if (removals.size > 0) {
> -        // Pick out sites with the same base domain as removed sites
> +          // Pick out sites with the same base domain as removed sites
> -        for (let site of this._sites) {
> +          for (let site of this._sites) {
> -          let baseDomain = Services.eTLD.getBaseDomainFromHost(site.host);
> +            let baseDomain = this._getBaseDomainFromHost(site.host);
> +            if (baseDomain) {
> -          let hosts = hostsTable.get(baseDomain);
> +              let hosts = hostsTable.get(baseDomain);
> -          if (hosts) {
> +              if (hosts) {
> -            hosts.push(site.host);
> +                hosts.push(site.host);
> -          }
> +              }
> -        }
> +            }

Likewise, we don't need to check the return value here or check whether the removals set has items.
Attachment #8868644 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8868644 [details]
Bug 1364852 - Handle error from Services.eTLD.getBaseDomainFromHost,

https://reviewboard.mozilla.org/r/140224/#review145218

> Instead, describe *why* we're making this change, that is, why these errors happen, and link to the bug that is supposed to deal with this in a more thorough way.

OK, updated, thanks.

> We don't want to return uninitialized, so please initialize to host.

OK, updated, thanks.

> Please just throw e;

OK, updated, thanks.

> Please remove the `finally` block and just return at the end of the function.

OK, updated, thanks.

> You can iterate over a set directly, ie you don't need the `.values()` bit.

OK, updated, thanks.

> We should never hit this case, so we can just not catch errors here.
> 
> In any case, removing items from the set while iterating over it likely doesn't work correctly anyway...

OK, updated, thanks.

> Likewise, we don't need to check the return value here or check whether the removals set has items.

OK, updated, thanks.
(In reply to Fischer [:Fischer] from comment #18)
> Comment on attachment 8868644 [details]
> Bug 1364852 - Handle error from Services.eTLD.getBaseDomainFromHost,
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/140224/diff/3-4/
TRY: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2457bee1ed3cd08ab0c5d3e9c7372d7bd004697c
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/2d3e3cdd9d65
Handle error from Services.eTLD.getBaseDomainFromHost, r=Gijs
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2d3e3cdd9d65
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: