Closed Bug 1398043 Opened 7 years ago Closed 7 years ago

Crash in mozilla::dom::cache::db::`anonymous namespace''::ReadResponse

Categories

(Core :: DOM: Core & HTML, defect)

57 Branch
Unspecified
Windows 10
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 --- fixed

People

(Reporter: calixte, Assigned: tt)

References

(Blocks 1 open bug)

Details

(Keywords: crash, regression, Whiteboard: [clouseau])

Crash Data

Attachments

(3 files)

This bug was filed from the Socorro interface and is 
report bp-15601f3b-dd9b-47ba-a544-134560170908.
=============================================================

There are 85 crashes in nightly 57 with buildid 20170907220212. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 1290481.

[1] https://hg.mozilla.org/mozilla-central/rev?node=3b635d9ede1b2bb834abd31a0a81ead928695bdf
Flags: needinfo?(ttung)
I'll investigate this
Assignee: nobody → ttung
Flags: needinfo?(ttung)
Crash Signature: [@ mozilla::dom::cache::db::`anonymous namespace''::ReadResponse] → [@ mozilla::dom::cache::db::`anonymous namespace''::ReadResponse] [@ mozilla::dom::cache::db::(anonymous namespace)::ReadResponse]
Could you please add a unit test?
Thanks
Hi Ben,

I guess found the silly problem and I can reproduce it on my desktop (ubuntu). There a syntax error of SQLite when updating schema 25 to 26 (forgetting to add space between strings). 

So that the update doesn't work (update the response_padding_size to 0 where type is opaque). And, it hits the assertion for ensuring response_padding_size shouldn't be null for opaque response when readResponse().

This patch tries to fix the original issue. I will write the other patch for extra updating response_padding_size to 0 if it's null and response type is opaque when reading a cached response.
(In reply to Sylvestre Ledru [:sylvestre] from comment #3)
> Could you please add a unit test?
> Thanks

Sure, I'll add a unit test. Sorry for making this crash.
Crash Signature: [@ mozilla::dom::cache::db::`anonymous namespace''::ReadResponse] [@ mozilla::dom::cache::db::(anonymous namespace)::ReadResponse] → [@ mozilla::dom::cache::db::`anonymous namespace''::ReadResponse] [@ mozilla::dom::cache::db::(anonymous namespace)::ReadResponse] [@ ReadResponse]
I can still see the crash with build ID 20170908100218 OS : win 10X64

latest crash report : https://crash-stats.mozilla.com/about/throttling
Comment on attachment 8905827 [details]
Bug 1398043 - P1: Add a space between strings concatenation.

https://reviewboard.mozilla.org/r/177634/#review182776

I'm surprised this wasn't caught by this test:

http://searchfox.org/mozilla-central/source/dom/cache/test/xpcshell/test_migration.js

Do you know how it got by there?
Attachment #8905827 - Flags: review?(bkelly) → review+
Comment on attachment 8905840 [details]
Bug 1398043 - P2: Temporary fix for updating the existing response_padding_size to 0 when it's an opaque response with null padding size.

https://reviewboard.mozilla.org/r/177646/#review182778

r=me with comments addressed.

::: dom/cache/DBSchema.cpp:2087
(Diff revision 4)
>  
>    bool nullPadding = false;
>    rv = state->GetIsNull(6, &nullPadding);
>    if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
>  
> +# ifndef RELEASE_OR_BETA

Is there a reason you used `#ifndef RELEASE_OR_BETA` instead of `#ifdef NIGHTLY_BUILD`?  I think the nightly macro is more precise and easier to read.

::: dom/cache/DBSchema.cpp:2090
(Diff revision 4)
>    if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
>  
> +# ifndef RELEASE_OR_BETA
> +  bool shouldUpdateTo26 = false;
> +  if (nullPadding && aSavedResponseOut->mValue.type() == ResponseType::Opaque) {
> +    // XXXtt: This should be removed in the future (e.g. Nightly 58).

Please file a follow-up bug for it.
Attachment #8905840 - Flags: review?(bkelly) → review+
Comment on attachment 8905897 [details]
Bug 1398043 - P3: Add a test to verify (DOM) Cache upgrade successfully.

https://reviewboard.mozilla.org/r/177702/#review182782

Thanks for adding the test!  I'm still curious why the existing migration test didn't catch it, though.
Attachment #8905897 - Flags: review?(bkelly) → review+
Could you please check the code coverage for this? Especially if we indeed have coverage or not. Gracie
Flags: needinfo?(mcastelluccio)
(In reply to Ben Kelly [:bkelly] from comment #14)
> Comment on attachment 8905827 [details]
> Bug 1398043 - P1: Add a space between strings concatenation.
> 
> https://reviewboard.mozilla.org/r/177634/#review182776
> 
> I'm surprised this wasn't caught by this test:
> 
> http://searchfox.org/mozilla-central/source/dom/cache/test/xpcshell/
> test_migration.js
> 
> Do you know how it got by there?
 
I use [1] to open the cache.sqlite in the test. I only found one entry and its response type is ResponseType::Default. Thus, this test doesn't catch this (Schema25To26 updates entries where response type is ResponseType::Opaque).

[1] https://sqliteonline.com/
Blocks: 1398167
(In reply to Ben Kelly [:bkelly] from comment #15)
> ::: dom/cache/DBSchema.cpp:2090
> (Diff revision 4)
> >    if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> >  
> > +# ifndef RELEASE_OR_BETA
> > +  bool shouldUpdateTo26 = false;
> > +  if (nullPadding && aSavedResponseOut->mValue.type() == ResponseType::Opaque) {
> > +    // XXXtt: This should be removed in the future (e.g. Nightly 58).
> 
> Please file a follow-up bug for it.

File bug 1398167 for it
Hmm, not sure I understand.  If the migration failed we should not be able to open the Cache since the schema version will not match the latest.
(In reply to Ben Kelly [:bkelly] from comment #20)
> Hmm, not sure I understand.  If the migration failed we should not be able
> to open the Cache since the schema version will not match the latest.

I'm not sure about this as well, but I can fix the issue which I reproduce by these patches. I'll keep investigating if there is any other things I forgot to take care.

Maybe mark leave-open to see if this fix really solve the problem?
Keywords: leave-open
(In reply to Tom Tung [:tt] (OoO: 9/22 ~ 9/29) from comment #21)
> I'm not sure about this as well, but I can fix the issue which I reproduce
> by these patches. I'll keep investigating if there is any other things I
> forgot to take care.

Somehow the syntax "0WHERE" makes our SQLite API neither return the error code nor execute the |UPDATE| successfully. Thus, the version is upgraded to 26 without update the entires...
Pushed by ttung@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/01ccedeeaf8a
P1: Add a space between strings concatenation. r=bkelly
https://hg.mozilla.org/integration/autoland/rev/60499cc10fc4
P2: Temporary fix for updating the existing response_padding_size to 0 when it's an opaque response with null padding size. r=bkelly
https://hg.mozilla.org/integration/autoland/rev/83878b29466c
P3: Add a test to verify (DOM) Cache upgrade successfully. r=bkelly
Good news: this was the #4 Windows topcrash in Nightly 20170908100218, but is absent in Nightly 20170908220146, which is the first Nightly that has the fixes.

> Maybe mark leave-open to see if this fix really solve the problem?

Looks like it has. I will close.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
See Also: → 1398649
(In reply to Sylvestre Ledru [:sylvestre] from comment #17)
> Could you please check the code coverage for this? Especially if we indeed
> have coverage or not. Gracie

The MigrateFrom25To26 function (modified with https://hg.mozilla.org/mozilla-central/rev/01ccedeeaf8a) is covered (https://codecov.io/gh/marco-c/gecko-dev/src/9c378ea156e4a49f1a2ad9ba3fdd47cb4ea35c28/dom/cache/DBSchema.cpp#L3174).
The other code added with https://hg.mozilla.org/mozilla-central/rev/60499cc10fc4 doesn't seem to be covered: https://codecov.io/gh/marco-c/gecko-dev/src/9c378ea156e4a49f1a2ad9ba3fdd47cb4ea35c28/dom/cache/DBSchema.cpp#L2089.
Flags: needinfo?(mcastelluccio)
(In reply to Marco Castelluccio [:marco] from comment #28)
> The MigrateFrom25To26 function (modified with
> https://hg.mozilla.org/mozilla-central/rev/01ccedeeaf8a) is covered
> (https://codecov.io/gh/marco-c/gecko-dev/src/
> 9c378ea156e4a49f1a2ad9ba3fdd47cb4ea35c28/dom/cache/DBSchema.cpp#L3174).
> The other code added with
> https://hg.mozilla.org/mozilla-central/rev/60499cc10fc4 doesn't seem to be
> covered:
> https://codecov.io/gh/marco-c/gecko-dev/src/
> 9c378ea156e4a49f1a2ad9ba3fdd47cb4ea35c28/dom/cache/DBSchema.cpp#L2089.

Yeah, indeed. The issue raised because I didn't handle upgrade properly in bug 1290481. It makes Nightly users upgrade their dom cache DB's version without update the SQLite table, and they hit the assertion when reading cached response from the table.

So, the code in [1] is mainly to make the users, who have not upgraded their dom cache DB, upgrade their dom cache DB properly in the future.

The code in [2] basically do the same thing as [1], and it's a temporary fix to help the Nightly users who have upgraded their dom cache DB improperly. That's why I add a #ifdef Nightly_Build flag for this. We will remove [2] after few weeks in bug 1398167. Thus, I'm not sure if I need to add a test to cover it. If so, we will remove it in bug 1398167 as well.

Note: I think we could uplift the code in [2] to 57 Beta in bug 1398167 as well if it's needed.

[1] https://hg.mozilla.org/mozilla-central/rev/01ccedeeaf8a
[2] https://hg.mozilla.org/mozilla-central/rev/60499cc10fc4
Keywords: leave-open
Target Milestone: --- → mozilla57
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.