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

RESOLVED FIXED in Firefox 57

Status

enhancement
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: kaie, Assigned: kaie)

Tracking

57 Branch
mozilla57

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(3 attachments, 2 obsolete attachments)

Assignee

Description

2 years ago
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.
Assignee

Updated

2 years ago
Target Milestone: mozilla56 → mozilla57
Version: 56 Branch → 57 Branch
Assignee

Updated

2 years ago
Attachment #8907048 - Attachment description: temporary patch for firefox, including documentation with removal plan → temporary patch for firefox 57, including documentation with removal plan
Assignee

Comment 3

2 years ago
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.
Assignee

Comment 4

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

Updated

2 years ago
See Also: → 1384633
Assignee

Comment 5

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

Updated

2 years ago
Attachment #8907044 - Flags: review?(mh+mozilla)
Assignee

Updated

2 years ago
Attachment #8907048 - Flags: review?(mh+mozilla)
Assignee

Comment 6

2 years ago
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.
Flags: needinfo?(mcmanus)
sgtm. thanks kai!
Flags: needinfo?(mcmanus)
Attachment #8907044 - Flags: review?(mh+mozilla) → review+
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.
Attachment #8907048 - Flags: review?(mh+mozilla)
Assignee

Comment 9

2 years ago
(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.
Assignee

Comment 10

2 years ago
Attachment #8907048 - Attachment is obsolete: true
Attachment #8908033 - Flags: review?(mh+mozilla)
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.
Attachment #8908033 - Flags: review?(mh+mozilla)
Assignee

Comment 12

2 years ago
(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.
Assignee

Comment 13

2 years ago
(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.
Assignee

Comment 14

2 years ago
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!
Attachment #8908033 - Attachment is obsolete: true
Attachment #8908563 - Flags: review?(mh+mozilla)
Assignee

Comment 15

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

Comment 16

2 years ago
Franziskus gave me r+ on IRC.
Assignee

Updated

2 years ago
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Attachment #8908563 - Flags: review?(mh+mozilla)
This breaks NSS uplifts. I'll attach a patch.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
NSS doesn't have a patch folder.
Attachment #8910146 - Flags: review?(kaie)
Assignee

Comment 22

2 years ago
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.
Attachment #8910146 - Flags: review?(kaie)
Assignee

Updated

2 years ago
Flags: needinfo?(franziskuskiefer)
> 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.
Flags: needinfo?(franziskuskiefer)

Comment 25

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/76aceb596872
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED

Updated

Last year
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.