1066726 - Concurrent cache read and write issues

Status: VERIFIED FIXED

(Core :: Networking: Cache, defect)

Description

[Honza Bambas (:mayhemer)]

This is a cover bug for bug 1064408, bug 1036939 and bug 1048348.  They are all pretty connected together, I will dup them all to this single bug and have a single patch for all of them here.

Comment 4

[Honza Bambas (:mayhemer)]

Attachment: v1

Patch description:
- bug 1064408 comment 15
- bug 1048348 comment 4

In general:
- when the output stream of the entry is closed with a fatal error, set state of the file to that error
- forbid opening input stream anymore (it's an atomic way to indicate the entry is in a bad shape and useless that http channel can handle well w/o any more changes needed)
- don't throw the entry on SetPredictedSize failure since it:
  - would crash (bug 1048348)
  - it wouldn't engage the interrupted concurrent write resume

The rest should be commented in the code and covered by tests


https://tbpl.mozilla.org/?tree=Try&rev=62da06db24ad

Comment 6

[Michal Novotny [:michal]]

Comment on attachment 8488773 [details] [diff] [review]
v1

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

::: netwerk/test/unit/test_cache2-28-concurrent_read_resumable_entry_size_zero.js
@@ +54,5 @@
> +
> +  var chan1 = make_channel(URL + "/content");
> +  chan1.asyncOpen(new ChannelListener(firstTimeThrough, null), null);
> +  var chan2 = make_channel(URL + "/content");
> +  chan2.asyncOpen(new ChannelListener(secondTimeThrough, null), null);

Is it always ensured that the second channel gets the cache entry before the entry is doomed by the first channel?

Comment 8

[Honza Bambas (:mayhemer)]

(In reply to Michal Novotny (:michal) from comment #6)
> Comment on attachment 8488773 [details] [diff] [review]
> v1
> 
> Review of attachment 8488773 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> :::
> netwerk/test/unit/test_cache2-28-concurrent_read_resumable_entry_size_zero.js
> @@ +54,5 @@
> > +
> > +  var chan1 = make_channel(URL + "/content");
> > +  chan1.asyncOpen(new ChannelListener(firstTimeThrough, null), null);
> > +  var chan2 = make_channel(URL + "/content");
> > +  chan2.asyncOpen(new ChannelListener(secondTimeThrough, null), null);
> 
> Is it always ensured that the second channel gets the cache entry before the
> entry is doomed by the first channel?

Definitely is, the second channel gets the entry (OnCheck/OnAvail calls) when the first channel fills the metadata on the entry (what happens on the main thread in OnStart).  Dooming happens later in OnData of chan1, by that time chan2 already pump-reads the entry.

Comment 9

[Honza Bambas (:mayhemer)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1bb80b72541f

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1bb80b72541f

Comment 11

[Honza Bambas (:mayhemer)]

Comment on attachment 8488773 [details] [diff] [review]
v1

Approval Request Comment
[Feature/regressing bug #]: cache2enable
[User impact if declined]: broken or missing web content delivery to user's screen even freshly loading from servers (no work around with clearing cache or so)
[Describe test coverage new/current, TBPL]: patch adds a bunch of tests checking all corner cases the patch is fixing ; otherwise deeply tested code (main HTTP web content load)
[Risks and why]: hard to say, this code is complex
[String/UUID change made/needed]: none

Comment 12

[Sylvestre Ledru [:Sylvestre]]

For QA, I think this can be tested using the 3 bugs listed in comment #0.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/83a473a0e907
https://hg.mozilla.org/releases/mozilla-beta/rev/8a1cffa4c130

Is this something that needs tracking for B2G v2.0 (b2g32)?

Comment 14

[Honza Bambas (:mayhemer)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #13)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/83a473a0e907
> https://hg.mozilla.org/releases/mozilla-beta/rev/8a1cffa4c130
> 
> Is this something that needs tracking for B2G v2.0 (b2g32)?

Probably yes.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

In that case, please nominate this patch for b2g32 approval :). Might want to nominate it for v2.0 blocking status as well if you feel it's warranted.

Comment 16

[Honza Bambas (:mayhemer)]

Comment on attachment 8488773 [details] [diff] [review]
v1

Comment 11

Comment 17

[Ada [:adalucinet]]

Reproduced with Nightly 2014-07-20 on Windows 7 64bit with STR from bug 1036939.
Verified as fixed with Firefox 33 beta 6 (Build ID: 20140922173023), latest Aurora 34.0a2 (Build ID: 20140923004002) and latest Nightly 35.0a1 (Build ID: 20140923030204) on Windows 7 64-bit, Linux 14.04 32-bit and Mac OS X 10.9.5 with STR from bug 1036939 and bug 1064408.
Also, in Socorro there are no crashes after fixing this issue for [@ mozilla::net::nsHttpChannel::OnDoneReadingPartialCacheEntry(bool*)] signature (bug 1048348): https://crash-stats.mozilla.com/report/list?signature=mozilla%3A%3Anet%3A%3AnsHttpChannel%3A%3AOnDoneReadingPartialCacheEntry%28bool%2A%29&product=Firefox&query_type=contains&range_unit=weeks&process_type=any&hang_type=any&date=2014-09-23+14%3A00%3A00&range_value=1#tab-reports

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/152c5094843c

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Backed out for bustage. Please rebase.
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/1bfb51a08f2e

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=37434&repo=mozilla-b2g32_v2_0

Comment 20

[Honza Bambas (:mayhemer)]

The patch is technically dependent on bug 1000338 that has been uplifted on 2014-10-05 14:46:11.  I think you can reland now (I will first locally check the build and let you know).

Comment 21

[Ryan VanderMeulen [:RyanVM]]

I tried to land both at the same time.

Comment 22

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #20)
> The patch is technically dependent on bug 1000338 that has been uplifted on
> 2014-10-05 14:46:11.  I think you can reland now (I will first locally check
> the build and let you know).

Err... the bug is actually bug 1013395.  That one is not worth an uplift, but the parts needed to compile are small enough, I'll backport them.  However, there is some risk this patch won't be stable (since it is completely untested) w/o all other patches landed on 33+, this is simply a disclaimer.

Comment 23

[Honza Bambas (:mayhemer)]

Attachment: v1 [REFERENCE] (for mozilla-b2g32_v2_0/not functioning)

This builds, but tests are consistently failing around the code this patch is touching - something I was afraid of.  If we want to backport this to b2g/32 we need more patches to go along with it.  I have to search for all bugs that we have fixed for 33 and select those needed to make this patch work (best all of them).

Comment 24

[Honza Bambas (:mayhemer)]

Attachment: Missing bugs for backport

This is the list of cache2 bugs landed on Beta and not on B2G/32.  Not sure we can just select few to make the patch for this bug safely back-portable...

Comment 25

[Honza Bambas (:mayhemer)]

Probably more a question for Bhavana.

Comment 26

[bhavana bajaj [:bajaj]]

(In reply to Honza Bambas (:mayhemer), not reading bugmail until Oct 28 from comment #24)
> Created attachment 8501348 [details]
> Missing bugs for backport
> 
> This is the list of cache2 bugs landed on Beta and not on B2G/32.  Not sure
> we can just select few to make the patch for this bug safely back-portable...

I am surprised, because everything that lands on beta should be backported to b2g32. Ryan , any thoughts here?

Comment 27

[Ryan VanderMeulen [:RyanVM]]

I'm not entirely sure how many of the ones in that list landed on 33+ vs. when 32 was on beta.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

OK, so basically the bottom 5 landed on 32 when it was still on beta. Everything from bug 1013638 (rev 8f03e0ef5809) and up landed on 33+. I can't help but think that a mass uplift isn't what we're going to want to do here given where we are in the B2G v2.0 release cycle.

So I guess that leaves either wontfixing this or figuring out what the minimum branch patch is that could be safely uplifted to the fix the bug. Honza, thoughts?

Comment 29

[Joshua Mitchell (Inactive)]

issue is resolved, removing outdated qa-wanted tag

Comment 31

[Honza Bambas (:mayhemer)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #28)
> OK, so basically the bottom 5 landed on 32 when it was still on beta.
> Everything from bug 1013638 (rev 8f03e0ef5809) and up landed on 33+. I can't
> help but think that a mass uplift isn't what we're going to want to do here
> given where we are in the B2G v2.0 release cycle.
> 
> So I guess that leaves either wontfixing this or figuring out what the
> minimum branch patch is that could be safely uplifted to the fix the bug.
> Honza, thoughts?

I think finding a minimum patch would be very complicated and risky.  I vote for all or nothing.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

Honza, you're the best-qualified person to weigh the risks of doing that in comparison to the severity of the bugs it'd fix. B2G v2.0 is at a point where we don't want to be taking any unnecessary risks, so what's your take on wontfixing vs. proceeding with a mass uplift?

Comment 33

[Honza Bambas (:mayhemer)]

I'm against the mass uplift.  Those patches are touching also a different code and there can easily be dependencies we are not aware of and totally break the stable b2g 2.0 branch.

There are two issue when this bug is left unfixed:
- when disk cache size is set to 0, there are some problems with showing favicons, images, sometimes pages (1036939 1064408 1067103); this is a non-standard setting that some people from some reason used to set to disable the cache
- null crash in nsHttpChannel::OnDoneReadingPartialCacheEntry

The first one can be left as a known issue because of the non-standard setting.

The second one can be worked around by one of the options:
1. we can null check and fail on null entry (can lead to a failed response and not sure of all other consequences)
2. we can disobey a single entry limit when doing a partial request (safe and easy to do)

I'll create a branch patch with the second solution (one line).

Comment 34

[Ryan VanderMeulen [:RyanVM]]

Thanks, sounds like the way to go :)

Comment 35

[Honza Bambas (:mayhemer)]

Attachment: for mozilla-b2g32_v2_0, v2

- mCacheEntry->SetPredictedDataSize not called for a partial response (avoid the crash, bug 1048348)
- concurrent cache reading completely disabled (explanation in the code comment)
- test_partial_response_entry_size_smart_shrink.js has been slightly modified to accept missing bug 237623 on this branch (comments in the test)

Comment 36

[Honza Bambas (:mayhemer)]

Comment on attachment 8517537 [details] [diff] [review]
for mozilla-b2g32_v2_0, v2

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=bee6dd0d450c

Comment 37

[Ryan VanderMeulen [:RyanVM]]

(In reply to Honza Bambas (:mayhemer) from comment #36)
> Comment on attachment 8517537 [details] [diff] [review]
> for mozilla-b2g32_v2_0, v2
> 
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=bee6dd0d450c

As bad as this looks, most of the noise is expected based on being an old branch trying to run tests it wasn't designed to run. That said, https://treeherder.mozilla.org/ui/logviewer.html#?job_id=2961089&repo=try looks real to me.

Comment 38

[Honza Bambas (:mayhemer)]

Idea: we could disable the disk cache when capacity is 0 (internally, disabling the cache completely is something else then setting the capacity to 0 bytes right now).  I'll provide a second patch here.

Comment 39

[Honza Bambas (:mayhemer)]

Comment on attachment 8517537 [details] [diff] [review]
for mozilla-b2g32_v2_0, v2

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): the new HTTP cache
User impact if declined: comment 33
Testing completed: tbpl, has own automated tests added
Risk to taking this patch (and alternatives if risky): low, it disables some not highly significant perfomance features of the new cache
String or UUID changes made by this patch: none

Comment 40

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8517537 [details] [diff] [review]
for mozilla-b2g32_v2_0, v2

Does this address comment 37?

Comment 41

[Honza Bambas (:mayhemer)]

Attachment: for mozilla-b2g32_v2_0, v2, improvement 1, v1

Consider cache completely disabled when the capacity is set to 0.  I play with an idea to add this patch to m-c as well, Michal, what would you think?

Comment 42

[Honza Bambas (:mayhemer)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #40)
> Comment on attachment 8517537 [details] [diff] [review]
> for mozilla-b2g32_v2_0, v2
> 
> Does this address comment 37?

Hmm.. not sure what was there to be addressed.  I don't know how run tests on the b2g32. is there a separate try repo for it?

Comment 43

[Ryan VanderMeulen [:RyanVM]]

You can push it to Try like any other branch. Just don't expect sane results from test suites that normally don't run on it.

As for what needs addressing:
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #37)
> That said, https://treeherder.mozilla.org/ui/logviewer.html#?job_id=2961089&repo=try
> looks real to me.

Comment 44

[Honza Bambas (:mayhemer)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #43)
> You can push it to Try like any other branch. Just don't expect sane results
> from test suites that normally don't run on it.
> 
> As for what needs addressing:
> (In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #37)
> > That said, https://treeherder.mozilla.org/ui/logviewer.html#?job_id=2961089&repo=try
> > looks real to me.

So, what is the problem?

Comment 45

[Ryan VanderMeulen [:RyanVM]]

Sorted it out on IRC.

Comment 46

[Honza Bambas (:mayhemer)]

I was confused by comment 37 - there is a real _issue_ in the run:

14635 10:33:56 INFO - 2589 INFO TEST-UNEXPECTED-FAIL | /tests/netwerk/test/mochitests/test_partially_cached_content.html | First response should exist
14636 10:33:56 INFO - 2590 INFO TEST-UNEXPECTED-FAIL | /tests/netwerk/test/mochitests/test_partially_cached_content.html | Second response should exist

going to look into it.

Comment 47

[Honza Bambas (:mayhemer)]

Comment on attachment 8517537 [details] [diff] [review]
for mozilla-b2g32_v2_0, v2

Dropping based on the last comment

Comment 48

[Michal Novotny [:michal]]

Comment on attachment 8517537 [details] [diff] [review]
for mozilla-b2g32_v2_0, v2

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

(In reply to Honza Bambas (:mayhemer) from comment #41)
> Created attachment 8519955 [details] [diff] [review]
> for mozilla-b2g32_v2_0, v2, improvement 1, v1
> 
> Consider cache completely disabled when the capacity is set to 0.  I play
> with an idea to add this patch to m-c as well, Michal, what would you think?

Agree.

Comment 49

[Honza Bambas (:mayhemer)]

Trying again also with the second patch:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c29e55c08865

Comment 50

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #49)
> Trying again also with the second patch:
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c29e55c08865

Guys, I don't know how to properly test this.  I need some help here.

Comment 51

[bhavana bajaj [:bajaj]]

(In reply to Honza Bambas (:mayhemer) from comment #50)
> (In reply to Honza Bambas (:mayhemer) from comment #49)
> > Trying again also with the second patch:
> > https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c29e55c08865
> 
> Guys, I don't know how to properly test this.  I need some help here.

NI some build experts here you may be able help debug the failures or provide input..

Comment 52

[Liz Henry (:lizzard) (relman/hg->git project)]

Marcia, is this something you're able to help with?

Comment 53

[Marcia Knous [:marcia]]

(In reply to Liz Henry :lizzard from comment #52)
> Marcia, is this something you're able to help with?

Not really. Guessing tedm is the best one to help here.

Comment 54

[(not currently active) Ted Mielczarek]

Unfortunately it looks like you're running into problems because the build automation has changed since that branch. I don't know if there's an easy way to make things work, maybe mshal has some idea.

Comment 55

[Michael Shal [:mshal]]

This is the same issue I'm trying to fix in bug 1102974. Unfortunately I don't think there's an easy way to fix it - I have to find the right patches to uplift. I'm having issues figuring out what those are though without causing other problems to surface :/

Comment 56

[Michael Shal [:mshal]]

Well, I think I was able to address the build issues at least. Can you try to rebase on top of b2g32 and see if it's any better? I'm not sure if the test job failures are related or some other issue though.

Comment 57

[Honza Bambas (:mayhemer)]

(In reply to Michael Shal [:mshal] from comment #56)
> Well, I think I was able to address the build issues at least. Can you try
> to rebase on top of b2g32 and see if it's any better? I'm not sure if the
> test job failures are related or some other issue though.

Thank you.  Here it is:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=65d165912d34

Comment 58

[Michael Shal [:mshal]]

(In reply to Honza Bambas (:mayhemer) from comment #57)
> Thank you.  Here it is:
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=65d165912d34

Looks like the builds are green, at least. Is that sufficient for you? I don't think we'll be able to get all the tests green on try, since it will always be using m-c's test configuration on try, not b2g32's. Some of them may be easy to address (for example, it looks like the android tests just need an updated tooltool manifest), but from what I can gather I don't think we'll get much further without a lot of effort.

Comment 59

[Ryan VanderMeulen [:RyanVM]]

Don't worry about Android builds/tests on b2g32. Completely unsupported anyway :)

Honza, this looks like a real failure? I see it on WinXP debug as well. Everything else looks safe to ignore.
https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3374596&repo=try