1542293 - Rewrite various nsHttpResponseHead::Get* to use Tokenizer

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect, P3)

Description

[Honza Bambas (:mayhemer)]

I'd love to rewrite all code that uses nsHttp::FindToken and remove that function if easy to do and actually productive, of course, in favor of using mozilla::Tokenizer everywhere.

these are a bit special. we reparse the Cache-control response header every time we want some bit out of it. I could see a lazy initialized parser here (via mozilla::Maybe<>, maybe) that will give the expected values via getter const methods, similarly to [1]. Maybe [1] could be generalized to parse response directives as well, now it's used only to parse request C-C.

[1] https://searchfox.org/mozilla-central/source/netwerk/protocol/http/CacheControlParser.h

Comment 1

[Khyati Agarwal]

Hi, I want to work on this. Could you guide me a little?

Comment 2

[Srujana Peddinti [:srujana121]]

Hey, I came here from that other bug(https://bugzilla.mozilla.org/show_bug.cgi?id=1189349) where you suggested me to work this newly filed one instead of that(Sorry for the late reply, I was occupied with other things). I have been looking at the code and this is what I have understood.

1. nsHttp::FindToken basically returns the first instance of a specific token that is between separators (ie <sep><token><sep>).
2. mozilla:: Tokenizer is basically a typedef of TTokenizer<char> in xpcom/ds/Tokenizer.h doesnt care at all for the separators so in case of HTTP requests we need to manuall check if the token is bounded by separator.

Am I correct?

Comment 3

[Honza Bambas (:mayhemer)]

no, and I think I filed the bug too generally.

This started when I implemented new Cache-control directive in bug 1536511. We have a parser for Cache-control directive already, used on few places. That class is derived from Tokenizer. I want this code to use this CacheControlParser class instead of repeated use of nsHttp::FindToken. It means to teach CacheControlParser few more directives. Great would be to add a new member mozilla::Maybe<CacheControlParser> in nsHttpResponseHead as a lazy parser to do it only once.

Comment 4

[Sonakshi Saxena]

Hey Honza!

So I was working on this and according to my understanding we need to make nsHttpResponseHead.cpp to use CacheControlParser similar to nsHttp.cpp. But I'm not sure what do you mean by this

Great would be to add a new member mozilla::Maybe<CacheControlParser> in nsHttpResponseHead as a lazy parser to do it only once.

By that, I believe that we can't simply replace nsHttp::FindToken with cacheControlRequest rather we'll have to call CacheControlParser cacheControlRequest every time, for which you are suggesting to use mozilla::Maybe<CacheControlParser> just once in nsHttpResponseHead.cpp. If that's right, then where would be a good place to declare mozilla::Maybe<CacheControlParser> for this?

Also, will it be fine if we could pass CacheControlParser& cacheControlRequest in all the Get functions, and update the .h file with class CacheControlParser under namespace net?

Would be glad if you could help me with this!
Thanks :)

Comment 5

[Sonakshi Saxena]

Hey Honza!
I wanted to discuss my approach and the issue that I am facing while resolving this bug. Would be great if you could help!

My approach to this issue is as follows-
In order to remove FindToken from nsHttpResponseHead::GetMaxAgeValue_locked, I have added a new function in the parser -

const char* CacheControlParser::HasToken(const char* input, const char* token,
                      const char* separators) {
  if (!input) return nullptr;

  int inputLen = strlen(input);
  int tokenLen = strlen(token);

  if (inputLen < tokenLen) return nullptr;

  const char* inputTop = input;
  const char* inputEnd = input + inputLen - tokenLen;
  for (; input <= inputEnd; ++input) {
    if (PL_strncasecmp(input, token, tokenLen) == 0) {
      if (input > inputTop && !strchr(separators, *(input - 1))) continue;
      if (input < inputEnd && !strchr(separators, *(input + tokenLen))) continue;
      return input;
    }
  }

  return nullptr;
}

Similarly, nsHttpResponseHead::GetMaxAgeValue_locked has been modified by adding -

nsAutoCString cacheControlRequestHeader;
Unused << GetHeader(nsHttp::X_Content_Type_Options, cacheControlRequestHeader);
CacheControlParser cacheControlRequest(cacheControlRequestHeader);

and FindToken is replaced by const char* p = cacheControlRequest.HasToken(val, "max-age", HTTP_HEADER_VALUE_SEPS "=");

Problem faced -
Since, GetMaxAgeValue_locked is a constant function, it is unable to call GetHeader within this file.

Please help me resolve this. Thanks! :)

Comment 6

[Sonakshi Saxena]

I guess I was proceeding in the wrong direction in the above comments. We might not need Maybe<CacheControlParser>, since the purpose of using it is that we parse the CacheControl header only once. And so, whenever we are setting that header, we are parsing it and storing it. I carefully read and understood the approach that you had mentioned earlier and have made a patch with the same.

Comment 7

[Sonakshi Saxena]

Attachment: Bug 1542293 - Rewrite various nsHttpResponseHead::Get* to use Tokenizer

Comment 8

[Honza Bambas (:mayhemer)]

ni? on me to push to try.

Comment 9

[Dragana Damjanovic [:dragana]]

Honza, can you finish up this? Is it ready to land?

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7d9103e6c1df
Rewrite various nsHttpResponseHead::Get* to use Tokenizer r=mayhemer

Comment 11

[Honza Bambas (:mayhemer)]

Thanks for the ping. Queued for landing.

Comment 12

[Cristian Brindusan [:cbrindusan]]

Backed out changeset 7d9103e6c1df (Bug 1542293) for xpcshell failures at test_race_cache_with_network.js.
https://hg.mozilla.org/integration/autoland/rev/6ea2ba32f5a1cdb0b89101e03b05a7c64abea944

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&revision=7d9103e6c1df0fcf3233a5b598c8a10ddcd420bd&selectedJob=297940153

Failure log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=297940153&repo=autoland&lineNumber=6567

Comment 13

[Sonakshi Saxena]

I'll check up on these errors.
Thanks!

Comment 14

[Honza Bambas (:mayhemer)]

This went through try few days ago w/o any problems: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c14170148339f7061bd9fc916d1ed4ec381d4180&searchStr=X2&selectedJob=296583401

Comment 15

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #14)

This went through try few days ago w/o any problems: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c14170148339f7061bd9fc916d1ed4ec381d4180&searchStr=X2&selectedJob=296583401

Ah!!! The error is different than the known intermittent reported bug, I must have overlooked that... OK, mea culpa, sorry for that!

Comment 16

[Honza Bambas (:mayhemer)]

Adding Michal who may help with this error.

Comment 17

[Sonakshi Saxena]

I looked up for the errors, and I'm not sure how to resolve them. But on digging up, I found some bugs related to the intermittent errors on this patch - Bug 1553718 and Bug 1307504, resolving them might help us

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/54cc2ea0b324
Rewrite various nsHttpResponseHead::Get* to use Tokenizer r=valentin,mayhemer,necko-reviewers

Comment 19

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/54cc2ea0b324