1399100 - Use a temporary Firefox-only patch on top of NSPR, to avoid exposing a temporary function

Status: RESOLVED FIXED

Description

[Kai Engert (:kaie:)]

Bug 1384633 had introduced a new API to NSPR.
We came to the conclusion that API isn't ideal and should be temporary.
Also, the API is supposed to be Windows-only.

I think it's not ideal that we introduce a new publicly exported symbol to a new NSPR release, even if it's just a temporary symbol.

I suggest these changes:

- in the official NSPR release, in nspr.def, 
  don't export PR_EXPERIMENTAL_ONLY_IN_4_17_GetOverlappedIOHandle

- use a downstream patch in Firefox code, that adds the
  export to nspr.def

I'll attach two patches, that make these changes to NSPR and to Firefox.

If accepted, I offer to land both changes into mozilla-inbound at the same time.

Comment 1

[Kai Engert (:kaie:)]

Attachment: remove the symbol from official NSPR

Comment 2

[Kai Engert (:kaie:)]

Attachment: temporary patch for firefox 57, including documentation with removal plan

Comment 3

[Kai Engert (:kaie:)]

Note the temporary patch also marks the symbol with version tag NSPR_4.17_fork to make it clear it's not an official API.

Comment 4

[Kai Engert (:kaie:)]

I cannot imagine problems with this approach, but let's be certain, try build:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=928472e9fc41fdb161fb6c3573852e176d824e11

Comment 5

[Kai Engert (:kaie:)]

I don't see any build failures or networking failures in the try build, only intermittent failures, that seem unrelated to this change.

Comment 6

[Kai Engert (:kaie:)]

Patrick, does the suggestion sound ok to you? Unfortunately both Dragana and Honza seem to be away currently, and the freeze for this NSPR release is supposed to be this week. Sorry for doing this cleanup late.

Comment 7

[Patrick McManus [:mcmanus]]

sgtm. thanks kai!

Comment 8

[Mike Hommey [:glandium]]

Comment on attachment 8907048 [details] [diff] [review]
temporary patch for firefox 57, including documentation with removal plan

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

::: nsprpub/patches/README.TXT
@@ +1,1 @@
> +bug1399100-temporary.patch :

A readme is not going to be noticed by someone who runs client.py to update nspr. You should put this in client.py output, or make client.py apply the patch.

Comment 9

[Kai Engert (:kaie:)]

(In reply to Mike Hommey [:glandium] from comment #8)
> 
> A readme is not going to be noticed by someone who runs client.py to update
> nspr. You should put this in client.py output, or make client.py apply the
> patch.

You are right that we need to add some protection.

I'm reluctant to introduce a mechanism that's fully automatic, as I don't want to encourage people to use this approach routinely.

Therefore I prefer a warning. The attached new patch moves the patches to a separate directory, which won't get removed by the update script. If there are any files inside the directory nsprpub.patches (when updating nspr) or inside the directory security/nss.patches (when updating nss), the script will print a warning, and require to press enter, allowing the operator of the script to notice.

Comment 10

[Kai Engert (:kaie:)]

Attachment: 1399100-firefox-temporary-v2.patch

Comment 11

[Mike Hommey [:glandium]]

Comment on attachment 8908033 [details] [diff] [review]
1399100-firefox-temporary-v2.patch

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

::: client.py
@@ +119,5 @@
>  
> +def warn_if_patch_exists(path):
> +    # If the given patch directory exists and contains at least one file,
> +    # then print warning and wait for the user to acknowledge.
> +    if os.path.exists(path) and os.path.isdir(path):

you only need the latter.

@@ +120,5 @@
> +def warn_if_patch_exists(path):
> +    # If the given patch directory exists and contains at least one file,
> +    # then print warning and wait for the user to acknowledge.
> +    if os.path.exists(path) and os.path.isdir(path):
> +        for root, dirs, files in os.walk(path):

You can just do something like if os.listdir(path)

::: nsprpub.patches/README.TXT
@@ +1,1 @@
> +bug1399100-temporary.patch :

Adding a directory at the toplevel for this is not really nice. The previous patch had a subdirectory of nsprpub, that's better.

Comment 12

[Kai Engert (:kaie:)]

(In reply to Mike Hommey [:glandium] from comment #11)
> 
> ::: nsprpub.patches/README.TXT
> @@ +1,1 @@
> > +bug1399100-temporary.patch :
> 
> Adding a directory at the toplevel for this is not really nice. The previous
> patch had a subdirectory of nsprpub, that's better.

But that subdirectory will be removed by the client.py update action, so the user cannot easily get access to the previous patch, without digging it out from history.

Comment 13

[Kai Engert (:kaie:)]

(In reply to Kai Engert (:kaie:) from comment #12)
> 
> But that subdirectory will be removed by the client.py update action, so the
> user cannot easily get access to the previous patch, without digging it out
> from history.

OK, I could change client.py to move the patch files to a temporary location, and then move them back to the subdirectory after the update has completed.

Comment 14

[Kai Engert (:kaie:)]

Attachment: 1399100-firefox-temporary-v3.patch

Thanks for your suggestions, I've addressed them.

Do you think this could land as is it is now, and have additional cleanup/tweaks done later? It would be great to get this landed and NSPR released prior to the next merge day. Thanks!

Comment 15

[Kai Engert (:kaie:)]

(Apparently merge day was last night. We'll have to ask for beta approval to get this landed.)

Comment 16

[Kai Engert (:kaie:)]

Franziskus gave me r+ on IRC.

Comment 17

[Kai Engert (:kaie:)]

Comment on attachment 8907044 [details] [diff] [review]
remove the symbol from official NSPR

https://hg.mozilla.org/projects/nspr/rev/64f9cb0f8b1c

Comment 18

[Kai Engert (:kaie:)]

Comment on attachment 8908563 [details] [diff] [review]
1399100-firefox-temporary-v3.patch

NSPR part:
https://hg.mozilla.org/integration/mozilla-inbound/rev/63281179bfe0

client.py:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f71235ab648

Comment 19

[Wes Kocher (:KWierso)]

https://hg.mozilla.org/mozilla-central/rev/63281179bfe0
https://hg.mozilla.org/mozilla-central/rev/6f71235ab648

Comment 20

[Franziskus Kiefer [:franziskus] (Away until October 2019)]

This breaks NSS uplifts. I'll attach a patch.

Comment 21

[Franziskus Kiefer [:franziskus] (Away until October 2019)]

Attachment: fix-client.py.patch

NSS doesn't have a patch folder.

Comment 22

[Kai Engert (:kaie:)]

Comment on attachment 8910146 [details] [diff] [review]
fix-client.py.patch

The first part looks good.

The second part needs a change. Because you have moved the permanent dir, it no longer exists, and we need to check for the temporary location.

   # move patch directory back to a subdirectory
-  shutil.move(temporary_patch_dir, permanent_patch_dir)
+  if os.path.exists(temporary_patch_dir):
+    shutil.move(temporary_patch_dir, permanent_patch_dir)

If you agree, r=kaie with that change.

Comment 23

[Franziskus Kiefer [:franziskus] (Away until October 2019)]

> The second part needs a change. Because you have moved the permanent dir, it no longer exists, and we need to check for the temporary location.

oops, indeed. I'll change that and land it.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/76aceb596872
fix client.py follow-up, r=kaie

Comment 25

[Wes Kocher (:KWierso)]

https://hg.mozilla.org/mozilla-central/rev/76aceb596872