Closed Bug 998372 Opened 6 years ago Closed 6 years ago

modernize the PAC FindProxyForURL function

Categories

(Testing :: Mozbase, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla31

People

(Reporter: froydnj, Assigned: froydnj)

Details

Attachments

(6 files, 2 obsolete files)

We can be more efficient and clearer, all at the same time.
We don't need to construct the regex on every call to the function.  We only
need to construct it once.
Attachment #8409017 - Flags: review?(jmaher)
Similarly to the regular expression from part 1, we can lift out the
construction of the known origins list.  While we're at it, let's rename it and
make it an object so we can do faster checks for known origins.
Attachment #8409019 - Flags: review?(jmaher)
We can do everything with some better-named tables and variables that (I think)
make it a little more obvious what's going on.
Attachment #8409020 - Flags: review?(jmaher)
Finally, this comment is just old, and what we have clearly works, so let's
just delete it.
Attachment #8409022 - Flags: review?(jmaher)
Comment on attachment 8409017 [details] [diff] [review]
part 1 - lift regex construction out of FindProxyForURL

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

thanks for the really small patch
Attachment #8409017 - Flags: review?(jmaher) → review+
Comment on attachment 8409019 [details] [diff] [review]
part 2 - lift origins out of FindProxyForURL and make origin lookups more efficient

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

::: netwerk/base/public/nsChannelProperties.h
@@ -31,3 @@
>  #define NS_CHANNEL_PROP_CHANNEL_POLICY \
>    NS_LITERAL_STRING(NS_CHANNEL_PROP_CHANNEL_POLICY_STR)
> -#endif

why is this in here?

::: testing/mozbase/mozprofile/mozprofile/permissions.py
@@ +308,5 @@
>          # to (\\\\d+) makes this code work. Not sure why there would be this
>          # difference between automation.py.in and this file.
>          pacURL = """data:text/plain,
> +var knownOrigins = (function () {
> +  return [%(origins)s].reduce(function(t, h) { t[h] = true; return t; }, {})

I don't really understand this.  are you just assigning true to all values h in t, not checking if it is true?
Attachment #8409019 - Flags: review?(jmaher) → review-
Comment on attachment 8409020 [details] [diff] [review]
part 3 - remove swathes of conditional logic from FindProxyForURL

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

this is nice and more clear!
Attachment #8409020 - Flags: review?(jmaher) → review+
Comment on attachment 8409022 [details] [diff] [review]
part 4 - remove old comment referring to automation.py.in

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

use your judgement

::: testing/mozbase/mozprofile/mozprofile/permissions.py
@@ -305,5 @@
>  
>          # TODO: this should live in a template!
> -        # TODO: So changing the 5th line of the regex below from (\\\\\\\\d+)
> -        # to (\\\\d+) makes this code work. Not sure why there would be this
> -        # difference between automation.py.in and this file.

I think this is valuable information to retain (sans the automation.py.in stuff)
Attachment #8409022 - Flags: review?(jmaher) → review+
(In reply to Joel Maher (:jmaher) from comment #6)
> ::: netwerk/base/public/nsChannelProperties.h
> @@ -31,3 @@
> >  #define NS_CHANNEL_PROP_CHANNEL_POLICY \
> >    NS_LITERAL_STRING(NS_CHANNEL_PROP_CHANNEL_POLICY_STR)
> > -#endif
> 
> why is this in here?

Because I am bad at separating patches.  I will remove it.

> ::: testing/mozbase/mozprofile/mozprofile/permissions.py
> @@ +308,5 @@
> >          # to (\\\\d+) makes this code work. Not sure why there would be this
> >          # difference between automation.py.in and this file.
> >          pacURL = """data:text/plain,
> > +var knownOrigins = (function () {
> > +  return [%(origins)s].reduce(function(t, h) { t[h] = true; return t; }, {})
> 
> I don't really understand this.  are you just assigning true to all values h
> in t, not checking if it is true?

Right.  I'm making each origin a key/property of an object, and setting all the corresponding values to |true|.  Then I can just check "is this thing a property" later.  I guess I could use a Set or similar, but this seemed like the easiest way to do things.
Here's part 2 again with the cruft removed.  Ideally the explanation I gave
previously will suffice.
Attachment #8409019 - Attachment is obsolete: true
Attachment #8409684 - Flags: review?(jmaher)
(In reply to Joel Maher (:jmaher) from comment #8)
> Comment on attachment 8409022 [details] [diff] [review]
> part 4 - remove old comment referring to automation.py.in
> 
> Review of attachment 8409022 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> use your judgement
> 
> ::: testing/mozbase/mozprofile/mozprofile/permissions.py
> @@ -305,5 @@
> >  
> >          # TODO: this should live in a template!
> > -        # TODO: So changing the 5th line of the regex below from (\\\\\\\\d+)
> > -        # to (\\\\d+) makes this code work. Not sure why there would be this
> > -        # difference between automation.py.in and this file.
> 
> I think this is valuable information to retain (sans the automation.py.in
> stuff)

If you remove the references to automation.py.in, then you just have this weird comment about changing escaping makes things work, standing alone, without any reference to the outside world.

However, the comment bugged me enough that I went to look at why this is so.  What happens currently is this:

1. Python reduces \\\\d to \\d when parsing the string.
2. We serialize the pref value via JSON (!) when outputting the prefs.  JSON serialization will escape backslashes in strings.  The string value therefore now has \\\\d.
3. The preferences reader handles backslash escaping when reading the string.  So we're down to \\d.
4. The JavaScript engine also handles backslash escaping when reading the string for constructing the regexp object.  So we have \d to pass to the regex engine's parser.
5. The regex engine's parser sees \d as the digit character class.

Previously, when using automation.py.in, we did the following:

1. Python reduces \\\\\\\\d to \\\\d when parsing the string.
2. We wrote the string values out to the prefs file directly, without any JSON serialization.  So we wrote \\\\d.
3. We can now pick up at step 3 in the sequence above.

I guess I could try to distill that down to a comment if you like?
Flags: needinfo?(jmaher)
I think a simple comment indicating the multiple layers of escaping would be helpful.
Flags: needinfo?(jmaher)
Comment on attachment 8409684 [details] [diff] [review]
part 2 - lift origins out of FindProxyForURL and make origin lookups more efficient

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

thanks!
Attachment #8409684 - Flags: review?(jmaher) → review+
OK, r? on the wording.
Attachment #8409022 - Attachment is obsolete: true
Attachment #8409693 - Flags: review?(jmaher)
Comment on attachment 8409693 [details] [diff] [review]
part 4 - explain the escape-fest in permissions.py better

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

thanks for the comment, that will help in the future.
Attachment #8409693 - Flags: review?(jmaher) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/005a3405219c
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a42f8424ccb
https://hg.mozilla.org/integration/mozilla-inbound/rev/4029dbea3291
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9abacaf5453

Committed with a small change to part 3: the checking for what proxy to use was originally done on the original scheme from the URL, but in the posted patch, the checking was being done on the scheme used for origin checking.  This change made several websocket-related tests fail, as they were being redirected to the HTTP proxy rather than the websocket proxy.  Fixed in the obvious way with descriptive variable names.
Flags: in-testsuite+
Assignee: nobody → nfroyd
Backout because we have tests that care about the exact format for FindProxyForURL and I assumed those test failures were just intermittents:

https://hg.mozilla.org/integration/mozilla-inbound/rev/8f28e657fcec
So we have tests for the exact format of FindProxyForURL.  This patch goes with
part 2 and changes the test to look for (part of) the new definition of
knownOrigins.  I didn't think it was worth adding the actual function arguments
to reduce().

I'll fold this before commit.
Attachment #8410296 - Flags: review?(jmaher)
The tests had checks for the conditional checks for what we should return as
our proxy.  To keep the same spirit, I just made the tests check for the
key:value entries in the proxyForScheme hashtable.

I'll fold this before commit.
Attachment #8410297 - Flags: review?(jmaher)
Comment on attachment 8410296 [details] [diff] [review]
test fix for origin rearrangement in part 2

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

::: testing/mozbase/mozprofile/tests/permissions.py
@@ +122,5 @@
>          self.assertEqual(len(user_prefs), 2)
>          self.assertEqual(user_prefs[0], ('network.proxy.type', 2))
>          self.assertEqual(user_prefs[1][0], 'network.proxy.autoconfig_url')
>  
> +        origins_decl = "var knownOrigins = (function () {  return ['http://mochi.test:8888', 'http://127.0.0.1:80', 'http://127.0.0.1:8888'].reduce"

are you missing a }) to close off the function and call?
Attachment #8410296 - Flags: review?(jmaher) → review-
(In reply to Joel Maher (:jmaher) from comment #20)
> Comment on attachment 8410296 [details] [diff] [review]
> test fix for origin rearrangement in part 2
> 
> Review of attachment 8410296 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: testing/mozbase/mozprofile/tests/permissions.py
> @@ +122,5 @@
> >          self.assertEqual(len(user_prefs), 2)
> >          self.assertEqual(user_prefs[0], ('network.proxy.type', 2))
> >          self.assertEqual(user_prefs[1][0], 'network.proxy.autoconfig_url')
> >  
> > +        origins_decl = "var knownOrigins = (function () {  return ['http://mochi.test:8888', 'http://127.0.0.1:80', 'http://127.0.0.1:8888'].reduce"
> 
> are you missing a }) to close off the function and call?

No.  I didn't think it'd be worth it to write out the entire thing.  I can do that if you like.
Attachment #8410297 - Flags: review?(jmaher) → review+
Comment on attachment 8410296 [details] [diff] [review]
test fix for origin rearrangement in part 2

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

resolved via irc, r+
Attachment #8410296 - Flags: review- → review+
You need to log in before you can comment on or make changes to this bug.