1396110 - Intermittent netwerk/test/unit_ipc/test_redirect_history_wrap.js,test_ext_api_permissions.js | application crashed [@ WatchdogManager::TimeSinceLastActiveContext()]

Status: RESOLVED FIXED

(Core :: XPConnect, defect, P5)

Description

[Treeherder Bug Filer]

Filed by: wkocher [at] mozilla.com

https://treeherder.mozilla.org/logviewer.html#?job_id=127811863&repo=autoland

https://queue.taskcluster.net/v1/task/IQ2cWnyZRRKzhxBNn7Fe5A/runs/0/artifacts/public/logs/live_backing.log

Comment 1

[Michal Novotny [:michal]]

This doesn't look like a bug in necko code.

Comment 2

[Intermittent Failures Robot]

1 failures in 939 pushes (0.001 failures/push) were associated with this bug in the last 7 days.   

Repository breakdown:
* autoland: 1

Platform breakdown:
* osx-10-10: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1396110&startday=2017-08-28&endday=2017-09-03&tree=all

Comment 3

[Intermittent Failures Robot]

3 failures in 924 pushes (0.003 failures/push) were associated with this bug in the last 7 days.   

Repository breakdown:
* autoland: 2
* try: 1

Platform breakdown:
* linux64: 2
* linux32-stylo-disabled: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1396110&startday=2017-09-04&endday=2017-09-10&tree=all

Comment 4

[Intermittent Failures Robot]

2 failures in 1032 pushes (0.002 failures/push) were associated with this bug in the last 7 days.    

Repository breakdown:
* autoland: 2

Platform breakdown:
* linux64: 1
* linux32: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1396110&startday=2017-09-11&endday=2017-09-17&tree=all

Comment 5

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

This showed up on the day it got added in bug 1376507. Blake, can you take a look at this, please?

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bug 1396110 - Fix races during watchdog shutdown.

Removing the context from the list and shutting down the watchdog have to
happen under the lock or we could race. I could fix this by making
TimeSinceLastActiveContext deal with the "no active context" case, but that
seems ugly. This patch makes shutdown atomic from the point of view of the
watchdog thread. Unfortunately, the layering is a bit odd. Bill, do you have a
better idea of how this should be done?

Review commit: https://reviewboard.mozilla.org/r/181932/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/181932/

Comment 7

[Intermittent Failures Robot]

5 failures in 943 pushes (0.005 failures/push) were associated with this bug in the last 7 days.    

Repository breakdown:
* autoland: 3
* try: 1
* mozilla-inbound: 1

Platform breakdown:
* osx-10-10: 2
* windows7-32: 1
* windows10-64-stylo-disabled: 1
* linux64: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1396110&startday=2017-09-18&endday=2017-09-24&tree=all

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Can we instead try to re-order things so that UnregisterContext happens after shutting down the watchdog stuff? That seems simpler.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8910514 [details]
Bug 1396110 - Fix races during watchdog shutdown.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/181932/diff/1-2/

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

That made everything much simpler.

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8910514 [details]
Bug 1396110 - Fix races during watchdog shutdown.

https://reviewboard.mozilla.org/r/181932/#review188492

::: js/xpconnect/src/XPCJSContext.cpp:88
(Diff revision 1)
> +    void forget()
> +    {
> +        MOZ_ASSERT(mWatchdog);
> +        mWatchdog = nullptr;
> +    }

This worries me. It means we could be in the scope of an AutoLockWatchdog without having the lock (if, for example, an inner AutoLockWatchdog took over the lock and then released it).

::: js/xpconnect/src/XPCJSContext.cpp:484
(Diff revision 2)
>  
>      // Lock lasts until we return
>      AutoLockWatchdog lock(self);
>  
>      MOZ_ASSERT(self->Initialized());
>      MOZ_ASSERT(!self->ShuttingDown());

While you're here, can you please remove this assertion? I don't think it's valid, and it has caused problems for me while testing some stuff. I have a patch to remove it, but I think it's just as easy to fold it in here.

As far as I can tell, it's possible for the main thread to quickly start up and then shutdown the watchdog. If that happens, the watchdog thread could start in a state where it's already supposed to shut down.

Comment 12

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Sorry, that first comment was for a previous version of the patch. I don't know why it appeared. Please ignore it.

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8910514 [details]
Bug 1396110 - Fix races during watchdog shutdown.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/181932/diff/2-3/

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6e145fde853d
Fix races during watchdog shutdown. r=billm

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/6e145fde853d

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you get a chance.

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8910514 [details]
Bug 1396110 - Fix races during watchdog shutdown.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1376507
[User impact if declined]: Possibly shutdown crashes.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Not yet (should be soon).
[Needs manual test from QE? If yes, steps to reproduce]:  No.
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: No.
[Why is the change risky/not risky?]: This is a pretty simple re-ordering of operations.
[String changes made/needed]: n/a

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8910514 [details]
Bug 1396110 - Fix races during watchdog shutdown.

Fix a shutdown issue, taking it.
Should be in 57b5

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c048148b2f2d