Kan-Ru Chen [:kanru] (UTC+9) Assignee
	Description • 8 years ago

      No description provided.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 7 • 8 years ago
mozreview-review

Comment on attachment 8781085 [details]
Bug 1295103 - Check OriginAttributes::PopulateFromSuffix return value in TabContext and ContentChild.

https://reviewboard.mozilla.org/r/71590/#review69182

with returning false, r+

::: dom/ipc/ContentChild.cpp:2440
(Diff revision 1)
>    // note we do not need to force mUserContextId to the default here because
>    // the permission manager does that internally.
>    nsAutoCString originNoSuffix;
>    PrincipalOriginAttributes attrs;
> -  attrs.PopulateFromOrigin(permission.origin, originNoSuffix);
> +  bool success = attrs.PopulateFromOrigin(permission.origin, originNoSuffix);
> +  NS_ENSURE_TRUE(success, true);

Shouldn't you return false here. I'd expect this kind of failure to be rather fatal.

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]
	Comment 8 • 8 years ago
mozreview-review

Comment on attachment 8781081 [details]
Bug 1295103 - Check OriginAttributes::PopulateFromSuffix return value in SerializedLoadContext..

https://reviewboard.mozilla.org/r/71580/#review69302

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]
	Comment 9 • 8 years ago
mozreview-review

Comment on attachment 8781081 [details]
Bug 1295103 - Check OriginAttributes::PopulateFromSuffix return value in SerializedLoadContext..

https://reviewboard.mozilla.org/r/71580/#review69304

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]
	Comment 10 • 8 years ago
mozreview-review

Comment on attachment 8781086 [details]
Bug 1295103 - Use MOZ_MUST_USE in OriginAttributes.

https://reviewboard.mozilla.org/r/71592/#review69306

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]
	Comment 11 • 8 years ago
mozreview-review

Comment on attachment 8781081 [details]
Bug 1295103 - Check OriginAttributes::PopulateFromSuffix return value in SerializedLoadContext..

https://reviewboard.mozilla.org/r/71582/#review69308

	Honza Bambas (:mayhemer)
	Comment 12 • 8 years ago
mozreview-review

Comment on attachment 8781083 [details]
Bug 1295103 - Check OriginAttributes::PopulateFromSuffix return value in nsCookieService.

https://reviewboard.mozilla.org/r/71586/#review70290

::: netwerk/cookie/nsCookieService.cpp:2805
(Diff revision 1)
>  
>      nsAutoCString suffix;
>      NeckoOriginAttributes attrs;
>      stmt->GetUTF8String(IDX_ORIGIN_ATTRIBUTES, suffix);
> -    attrs.PopulateFromSuffix(suffix);
> +    if (!attrs.PopulateFromSuffix(suffix)) {
> +      return;

why return and not continue?

I think if this fails, the database might be corrupted?  ehsan knows this code better.

	Honza Bambas (:mayhemer)
	Comment 13 • 8 years ago
mozreview-review

Comment on attachment 8781082 [details]
Bug 1295103 - Check OriginAttributes::PopulateFromSuffix return value in DOMStorageDBThread

https://reviewboard.mozilla.org/r/71584/#review70288

	Honza Bambas (:mayhemer)
	Comment 14 • 8 years ago
mozreview-review

Comment on attachment 8781084 [details]
Bug 1295103 - Check OriginAttributes::PopulateFromSuffix return value in PackagedAppVerifier.

https://reviewboard.mozilla.org/r/71588/#review70294

	Kan-Ru Chen [:kanru] (UTC+9) Assignee
	Comment 15 • 8 years ago

(In reply to Honza Bambas (:mayhemer) from comment #12)
> Comment on attachment 8781083 [details]
> Bug 1295103 - Check OriginAttributes::PopulateFromSuffix return value in
> nsCookieService.
> 
> https://reviewboard.mozilla.org/r/71586/#review70290
> 
> ::: netwerk/cookie/nsCookieService.cpp:2805
> (Diff revision 1)
> >  
> >      nsAutoCString suffix;
> >      NeckoOriginAttributes attrs;
> >      stmt->GetUTF8String(IDX_ORIGIN_ATTRIBUTES, suffix);
> > -    attrs.PopulateFromSuffix(suffix);
> > +    if (!attrs.PopulateFromSuffix(suffix)) {
> > +      return;
> 
> why return and not continue?
> 
> I think if this fails, the database might be corrupted?  ehsan knows this
> code better.

Actually now I'm not sure what to do here. Looks like we should at least call HandleCorruptDB. Ehsan, what should I do to handle the case if OriginAttributes is not valid?

	(no longer active)
	Comment 16 • 8 years ago

I commented on ReviewBoard but it is not reflected here:

Hmm, what are the cases in whcih we'd fail to parse the OA?  The only reasonable case I can think of is when you open a cookie DB from a newer version of Firefox which has added an origin attribute in an older version which doesn't support that attribute, which would cause PopulateFromSuffixIterator::URLParamsIterator() to return false.  I don't think it's reasonable to trigger a DB rebuild in that case!

The only thing that would sort of make sense in that scenario is to ignore the OA attributes that we don't support, which basically amounts to not checking the return value here.

	Kan-Ru Chen [:kanru] (UTC+9) Assignee
	Comment 17 • 8 years ago

(In reply to :Ehsan Akhgari from comment #16)
> I commented on ReviewBoard but it is not reflected here:
> 
> Hmm, what are the cases in whcih we'd fail to parse the OA?  The only
> reasonable case I can think of is when you open a cookie DB from a newer
> version of Firefox which has added an origin attribute in an older version
> which doesn't support that attribute, which would cause
> PopulateFromSuffixIterator::URLParamsIterator() to return false.  I don't
> think it's reasonable to trigger a DB rebuild in that case!
> 
> The only thing that would sort of make sense in that scenario is to ignore
> the OA attributes that we don't support, which basically amounts to not
> checking the return value here.

Thanks, Ehsan! I will go the not checking the return value route.

	Pulsebot
	Comment 30 • 8 years ago

Pushed by kchen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/83fe5575f4bb
Check OriginAttributes::PopulateFromSuffix return value in SerializedLoadContext.. r=allstars
https://hg.mozilla.org/integration/autoland/rev/1e0a07384496
Check OriginAttributes::PopulateFromSuffix return value in DOMStorageDBThread r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/f1df7e0face1
Check OriginAttributes::PopulateFromSuffix return value in nsCookieService. r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/4457d1e15ac8
Check OriginAttributes::PopulateFromSuffix return value in PackagedAppVerifier. r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/22a24db4bb36
Check OriginAttributes::PopulateFromSuffix return value in TabContext and ContentChild. r=smaug
https://hg.mozilla.org/integration/autoland/rev/f11c01bf62a1
Use MOZ_MUST_USE in OriginAttributes. r=allstars

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 31 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/83fe5575f4bb
https://hg.mozilla.org/mozilla-central/rev/1e0a07384496
https://hg.mozilla.org/mozilla-central/rev/f1df7e0face1
https://hg.mozilla.org/mozilla-central/rev/4457d1e15ac8
https://hg.mozilla.org/mozilla-central/rev/22a24db4bb36
https://hg.mozilla.org/mozilla-central/rev/f11c01bf62a1