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

RESOLVED FIXED in Firefox 57

Status

()

P5
critical
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: intermittent-bug-filer, Assigned: mrbkap)

Tracking

({crash, intermittent-failure})

unspecified
mozilla58
crash, intermittent-failure
Points:
---

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 unaffected, firefox56 unaffected, firefox57 fixed, firefox58 fixed)

Details

(crash signature)

Attachments

(1 attachment)

This doesn't look like a bug in necko code.
Component: Networking → XPConnect
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Summary: Intermittent netwerk/test/unit_ipc/test_redirect_history_wrap.js | application crashed [@ WatchdogManager::TimeSinceLastActiveContext()] → Intermittent netwerk/test/unit_ipc/test_redirect_history_wrap.js,test_ext_api_permissions.js | application crashed [@ WatchdogManager::TimeSinceLastActiveContext()]
This showed up on the day it got added in bug 1376507. Blake, can you take a look at this, please?
Blocks: 1376507
Flags: needinfo?(mrbkap)
Assignee: nobody → mrbkap
Flags: needinfo?(mrbkap)
Comment hidden (mozreview-request)
Comment hidden (Intermittent Failures Robot)
Can we instead try to re-order things so that UnregisterContext happens after shutting down the watchdog stuff? That seems simpler.
Flags: needinfo?(mrbkap)
Comment hidden (mozreview-request)
That made everything much simpler.
Flags: needinfo?(mrbkap)
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.
Attachment #8910514 - Flags: review?(wmccloskey) → review+
Sorry, that first comment was for a previous version of the patch. I don't know why it appeared. Please ignore it.
Comment hidden (mozreview-request)

Comment 14

2 years ago
Pushed by mrbkap@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6e145fde853d
Fix races during watchdog shutdown. r=billm
https://hg.mozilla.org/mozilla-central/rev/6e145fde853d
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Please request Beta approval on this when you get a chance.
status-firefox55: --- → unaffected
status-firefox56: --- → unaffected
status-firefox57: --- → affected
status-firefox-esr52: --- → unaffected
Flags: needinfo?(mrbkap)
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
Flags: needinfo?(mrbkap)
Attachment #8910514 - Flags: approval-mozilla-beta?
Comment on attachment 8910514 [details]
Bug 1396110 - Fix races during watchdog shutdown.

Fix a shutdown issue, taking it.
Should be in 57b5
Attachment #8910514 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 19

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/c048148b2f2d
status-firefox57: affected → fixed
You need to log in before you can comment on or make changes to this bug.