Intermittent browser/components/sessionstore/test/browser_forget_async_closings.js | application crashed [@ SetLinkFlags + 0x15]

RESOLVED FIXED in Firefox 55

Status

()

Core
General
--
critical
RESOLVED FIXED
6 months ago
5 months ago

People

(Reporter: Treeherder Bug Filer, Assigned: wcpan)

Tracking

({crash, intermittent-failure})

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

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox54 unaffected, firefox55 fixed, firefox56 fixed)

Details

(crash signature)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

6 months ago
treeherder
Filed by: cbook [at] mozilla.com

https://treeherder.mozilla.org/logviewer.html#?job_id=110302795&repo=mozilla-central

https://queue.taskcluster.net/v1/task/MXZ-oioaT7CqTE1Eo-ZkIg/runs/0/artifacts/public/logs/live_backing.log
Created attachment 8881754 [details]
crash stack

Comment 2

6 months ago
shell32.dll!SetLinkFlags + 0x15
…
shell32.dll!CDestinationList::CommitList() + 0x77
xul.dll!mozilla::widget::JumpListBuilder::DoCommitListBuild(RefPtr<mozilla::widget::detail::DoneCommitListBuildCallback>) [JumpListBuilder.cpp:217b7fcf5894 : 494 + 0x9]

Is this related to bug 1354143's change? I see dmose also noted that bug from bug 1372597. Looks like activity-stream when enabled tickles things in the wrong way as it happens to load its init near sessionrestore time.
Flags: needinfo?(wpan)
Flags: needinfo?(florian)

Comment 3

6 months ago
As of latest mozilla-central from ~30 minutes ago, seems like it should consistently crash on try Windows 7 VM opt and debug when preffing on activity stream via +pref("browser.newtabpage.activity-stream.enabled", true); and running mochitests that include browser/components/sessionstore/test/browser_forget_async_closings.js.

exit code 1 crashed [@ SetLinkFlags + 0x15]
Windows 7 VM opt tc-M: https://treeherder.mozilla.org/logviewer.html#?job_id=110648394&repo=try&lineNumber=2889
Windows 7 VM debug tc-M: https://treeherder.mozilla.org/logviewer.html#?job_id=110646783&repo=try&lineNumber=2898

and exit code 3221226356
Windows 7 VM opt M: https://treeherder.mozilla.org/logviewer.html#?job_id=110650614&repo=try&lineNumber=4029
Windows 7 VM debug tc-M-e10s: https://treeherder.mozilla.org/logviewer.html#?job_id=110646814&repo=try&lineNumber=3460
(In reply to Ed Lee :Mardak from comment #2)
> shell32.dll!SetLinkFlags + 0x15
> …
> shell32.dll!CDestinationList::CommitList() + 0x77
> xul.dll!mozilla::widget::JumpListBuilder::DoCommitListBuild(RefPtr<mozilla::
> widget::detail::DoneCommitListBuildCallback>)
> [JumpListBuilder.cpp:217b7fcf5894 : 494 + 0x9]
> 
> Is this related to bug 1354143's change?

Seems likely.

I reviewed only the JS changes there, jimm was the reviewer for the C++ changes, forwarding the needinfo.
Blocks: 1354143
Flags: needinfo?(florian) → needinfo?(jmathies)

Comment 5

5 months ago
1 failures in 718 pushes (0.001 failures/push) were associated with this bug in the last 7 days.   

Repository breakdown:
* mozilla-central: 1

Platform breakdown:
* windows7-32-vm: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1376760&startday=2017-06-26&endday=2017-07-02&tree=all
Got a Win7 machine, will trying to figure out what happened.
Flags: needinfo?(wpan)
I suspect we can avoid this crash by using CoInitializeEx(nullptr, COINIT_MULTITHREADED).
It will intermittently crash before, but works fine after initialized as multi-threaded, both in 30 runs.

Comment 8

5 months ago
Thanks so much, wcp!  This is turning out to be related to (or the same as?) a blocker for turning on Activity Stream in Nightly; looking forward to seeing it land...

Updated

5 months ago
Duplicate of this bug: 1376924

Updated

5 months ago
status-firefox54: --- → unaffected
status-firefox55: --- → affected
status-firefox56: --- → affected
Flags: needinfo?(jmathies)

Updated

5 months ago
Crash Signature: [@ SetLinkFlags + 0x15] → [@ SetLinkFlags + 0x15] [@ SetLinkFlags]
Ok, it still crashed when I repeatedly run the test 300 times.
I'll try to add a ReentrantMonitor for it to see if that helps.
Adding the lock survives after 300 runs.
Trying on try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=eec6c4909fe088ad849f230f2c58673df6b719a8
Comment hidden (mozreview-request)

Comment 13

5 months ago
mozreview-review
Comment on attachment 8884180 [details]
Bug 1376760 - Fix race condition for ICustomDestinationList usage.

https://reviewboard.mozilla.org/r/155106/#review160492

::: widget/windows/JumpListBuilder.cpp:131
(Diff revision 1)
>    }
>  }
>  
>  JumpListBuilder::~JumpListBuilder()
>  {
> +  ReentrantMonitorAutoEnter lock(mMonitor);

do we need this in the destructor?
Attachment #8884180 - Flags: review?(jmathies) → review+

Comment 14

5 months ago
2 failures in 656 pushes (0.003 failures/push) were associated with this bug in the last 7 days.   

Repository breakdown:
* try: 1
* mozilla-central: 1

Platform breakdown:
* windows7-32-vm: 2

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1376760&startday=2017-07-03&endday=2017-07-09&tree=all
(In reply to Jim Mathies [:jimm] from comment #13)
> ::: widget/windows/JumpListBuilder.cpp:131
> (Diff revision 1)
> >    }
> >  }
> >  
> >  JumpListBuilder::~JumpListBuilder()
> >  {
> > +  ReentrantMonitorAutoEnter lock(mMonitor);
> 
> do we need this in the destructor?

Probably not. The only possible race (as for now) is the lazy idle thread, and when it is using mJumpList, the refcnt will never be 0.
Comment hidden (mozreview-request)
Assignee: nobody → wpan

Comment 17

5 months ago
Pushed by wpan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/172282c86233
Fix race condition for ICustomDestinationList usage. r=jimm

Comment 18

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/172282c86233
Status: NEW → RESOLVED
Last Resolved: 5 months ago
status-firefox56: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Please request Beta approval on this when you get a chance.
status-firefox-esr52: --- → unaffected
Flags: needinfo?(wpan)
Just in case, I'd like to wait one or two days to see if this bug still happens on m-c.
Duplicate of this bug: 1372597
Comment on attachment 8884180 [details]
Bug 1376760 - Fix race condition for ICustomDestinationList usage.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1354143
[User impact if declined]: The crash rate could be higher than before.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Added a recursive mutex lock only, no control flow changed.
[String changes made/needed]: None.
Flags: needinfo?(wpan)
Attachment #8884180 - Flags: approval-mozilla-beta?
Comment on attachment 8884180 [details]
Bug 1376760 - Fix race condition for ICustomDestinationList usage.

add lock around JumpListBuilder, beta55+
Attachment #8884180 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 24

5 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/789c8f561602
status-firefox55: affected → fixed
You need to log in before you can comment on or make changes to this bug.