If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Use ScopeExit in a few more places

RESOLVED FIXED in Firefox 54

Status

()

Core
JavaScript Engine
RESOLVED FIXED
9 months ago
8 months ago

People

(Reporter: sfink, Assigned: sfink)

Tracking

unspecified
mozilla54
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

9 months ago
Going through some old patches, I found this one where I was beefing up MakeScopeExit with another feature (enable/disable) and using it in a couple of places. But I noticed that some of the changes didn't even require the new feature. And the ones that did weren't very compelling. So I ditched the new feature, and will try to land this small patch with a couple of new uses.
(Assignee)

Comment 1

9 months ago
Created attachment 8822095 [details] [diff] [review]
Use ScopeExit in a few more places

It uses some weird new syntactical goop, but it seems harder to mess up.
Attachment #8822095 - Flags: review?(luke)

Comment 2

9 months ago
Comment on attachment 8822095 [details] [diff] [review]
Use ScopeExit in a few more places

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

r+ to landing the WasmSignalHandlers.cpp changes.

::: js/src/builtin/AtomicsObject.cpp
@@ +996,5 @@
>          auto sliceEnd = finalEnd.map([&](mozilla::TimeStamp& finalEnd) {
>              auto sliceEnd = mozilla::TimeStamp::Now() + maxSlice;
>              if (finalEnd < sliceEnd)
>                  sliceEnd = finalEnd;
>              return sliceEnd;

IIUC, this return path would not have set the state_ to Idle before this patch but will set it to Idle with the patch.  I don't know what the intended behavior is here (and why are we returning a time value as a boolean....); probably best to split off a separate patch and have Lars review.
Attachment #8822095 - Flags: review?(luke) → review+
(Assignee)

Comment 3

9 months ago
(In reply to Luke Wagner [:luke] from comment #2)
> ::: js/src/builtin/AtomicsObject.cpp
> @@ +996,5 @@
> >          auto sliceEnd = finalEnd.map([&](mozilla::TimeStamp& finalEnd) {
> >              auto sliceEnd = mozilla::TimeStamp::Now() + maxSlice;
> >              if (finalEnd < sliceEnd)
> >                  sliceEnd = finalEnd;
> >              return sliceEnd;
> 
> IIUC, this return path would not have set the state_ to Idle before this
> patch but will set it to Idle with the patch.  I don't know what the
> intended behavior is here (and why are we returning a time value as a
> boolean....); probably best to split off a separate patch and have Lars
> review.

Heh. This isn't doing at all what you're thinking it does. That's not a return from the function (which, as you say, would be nonsense since the function returns a bool). It is just a return in the lambda passed to finalEnd.map. So this code isn't going to change state_ at all.

But sure, I'll toss it over to Lars.

And... hm. This is the first I've seen Maybe::map used. It's, uh, weird. Simpler example:

    auto finalEnd = timeout.map([](mozilla::TimeDuration& timeout) {
        return mozilla::TimeStamp::Now() + timeout;
    });

It's a simple assignment (finalEnd = Now() + timeout) with Nothing-ness propagation.

Gonna have to keep those darn Rustaceans off *my* lawn.
(Assignee)

Updated

9 months ago
Attachment #8822095 - Flags: review?(lhansen)

Comment 4

9 months ago
Ah hah, right, I missed the lambda.

Comment 5

9 months ago
Comment on attachment 8822095 [details] [diff] [review]
Use ScopeExit in a few more places

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

Nice!
Attachment #8822095 - Flags: review?(lhansen) → review+

Comment 6

8 months ago
Pushed by sfink@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/60fa55d96e2c
Use ScopeExit in a few more places, r=luke,lth

Comment 7

8 months ago
I had to back this out for build bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=71702688&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/2781e8137927e3970b142807a47d614650611966
Flags: needinfo?(sphink)

Comment 8

8 months ago
Pushed by sfink@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/85fec0c2e037
Use ScopeExit in a few more places, r=luke,lth
(Assignee)

Comment 9

8 months ago
Yes, there was an osx-only build failure. Which is weird, because I kind of agree with the osx compiler here.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fda28e136ab610d57882e45e65e9e5a191b8c18e seems to say I fixed that problem, though I probably rebased onto a bad rev for SM(arm).
Flags: needinfo?(sphink)

Comment 10

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/85fec0c2e037
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.