Enable Firefox Monitor by default

RESOLVED FIXED in Firefox 68

Status

()

enhancement
RESOLVED FIXED
4 months ago
2 months ago

People

(Reporter: nhnt11, Assigned: nhnt11)

Tracking

unspecified
Firefox 68
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox67+ wontfix, firefox68 fixed)

Details

Attachments

(5 attachments)

Assignee

Description

4 months ago

We've been using a Normandy recipe to flip the pref and enable the add-on for 100% of release users.

Now that this is in the tree, we should pref it on.

Assignee

Updated

3 months ago
Assignee: nobody → nhnt11
Status: NEW → ASSIGNED
Assignee

Comment 2

3 months ago

Johann, I've been postponing this preffing-on patch because we still haven't gotten data-review. Unfortunately it's been a bit difficult to sync with Sandy on the data-review request doc and get it checked out.

I have a meeting scheduled with her today though, and hopefully we can get data-review+ in the next couple of days. We'll have to uplift the patch to enable Telemetry - we should land this along with that and uplift them to beta together.

I prefer not to do uplifts like this, but I think it's unavoidable in this case. I think the risk is minimal since this has been QA'd and we have been shipping on Release channel via Go Faster and Normandy for a few releases now.

As the standing peer reviewer for Firefox Monitor code, I wanted to make sure you had a chance to comment on the situation. Thanks!

Flags: needinfo?(jhofmann)

Yeah, considering that 100% of release has this, I think the more dangerous course of action is actually to leave it preffed off in Beta and Nightly, thus getting less exposure with the audience that actually files bugs. The main idea of the pre-release channels is to protect release users from regressions and there's no risk in that here.

Thanks for checking, though!

Flags: needinfo?(jhofmann)

Comment 4

3 months ago
Pushed by nhnt11@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/7d5b25ce9e8f
Enable Firefox Monitor by default. r=johannh

Comment 5

3 months ago
Backout by rgurzau@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bd4178c6c695
Backed out changeset 7d5b25ce9e8f for osx mochitest failures on a CLOSED TREE.

Backed out changeset 7d5b25ce9e8f (bug 1531838)for osx mochitest failures on a CLOSED TREE.

Backout link: https://hg.mozilla.org/integration/autoland/rev/bd4178c6c6950034712acf97637cf73326139a62

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=7d5b25ce9e8f12276156ac98b7e46a0c71863a49&selectedJob=236799633

Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=236799633&repo=autoland&lineNumber=2251

Log snippet:

[task 2019-03-29T03:08:36.635Z] 03:08:36 INFO - Entering test bound test1b
[task 2019-03-29T03:08:36.635Z] 03:08:36 INFO - TEST-PASS | browser/base/content/test/plugins/browser_private_clicktoplay.js | Test 1b, Should have a click-to-play notification -
[task 2019-03-29T03:08:36.635Z] 03:08:36 INFO - TEST-PASS | browser/base/content/test/plugins/browser_private_clicktoplay.js | Test 1b, Plugin should not be activated - true == true -
[task 2019-03-29T03:08:36.635Z] 03:08:36 INFO - TEST-PASS | browser/base/content/test/plugins/browser_private_clicktoplay.js | 'Remember' checkbox should be hidden in private windows -
[task 2019-03-29T03:08:36.636Z] 03:08:36 INFO - Leaving test bound test1b
[task 2019-03-29T03:08:36.636Z] 03:08:36 INFO - Entering test bound test2a
[task 2019-03-29T03:08:36.636Z] 03:08:36 INFO - TEST-PASS | browser/base/content/test/plugins/browser_private_clicktoplay.js | Test 2a, Should have a click-to-play notification -
[task 2019-03-29T03:08:36.636Z] 03:08:36 INFO - TEST-PASS | browser/base/content/test/plugins/browser_private_clicktoplay.js | Test 2a, Plugin should not be activated - true == true -
[task 2019-03-29T03:08:36.636Z] 03:08:36 INFO - Leaving test bound test2a
[task 2019-03-29T03:08:36.637Z] 03:08:36 INFO - Entering test bound test2c
[task 2019-03-29T03:08:36.637Z] 03:08:36 INFO - Console message: [JavaScript Warning: "Array.forEach is deprecated; use Array.prototype.forEach instead" {file: "chrome://global/content/bindings/tabbox.xml" line: 162}]
[task 2019-03-29T03:08:36.638Z] 03:08:36 INFO - Console message: [JavaScript Warning: "Array.unshift is deprecated; use Array.prototype.unshift instead" {file: "chrome://browser/content/tabbrowser.js" line: 4821}]
[task 2019-03-29T03:08:36.638Z] 03:08:36 INFO - Console message: [JavaScript Warning: "Array.filter is deprecated; use Array.prototype.filter instead" {file: "chrome://browser/content/tabbrowser.js" line: 239}]
[task 2019-03-29T03:08:36.638Z] 03:08:36 INFO - Buffered messages logged at 03:08:36
[task 2019-03-29T03:08:36.639Z] 03:08:36 INFO - TEST-PASS | browser/base/content/test/plugins/browser_private_clicktoplay.js | should have created a private window. -
[task 2019-03-29T03:08:36.639Z] 03:08:36 INFO - loaded http://127.0.0.1:8888/browser/browser/base/content/test/plugins/plugin_test.html
[task 2019-03-29T03:08:36.641Z] 03:08:36 INFO - Buffered messages finished
[task 2019-03-29T03:08:36.642Z] 03:08:36 INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/plugins/browser_private_clicktoplay.js | Test 2c, Should have a click-to-play notification -
[task 2019-03-29T03:08:36.642Z] 03:08:36 INFO - Stack trace:
[task 2019-03-29T03:08:36.642Z] 03:08:36 INFO - chrome://mochikit/content/browser-test.js:test_ok:1304
[task 2019-03-29T03:08:36.642Z] 03:08:36 INFO - chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_private_clicktoplay.js:test2c:114
[task 2019-03-29T03:08:36.642Z] 03:08:36 INFO - chrome://mochikit/content/browser-test.js:Tester_execTest/<:1106
[task 2019-03-29T03:08:36.643Z] 03:08:36 INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1134
[task 2019-03-29T03:08:36.646Z] 03:08:36 INFO - chrome://mochikit/content/browser-test.js:nextTest/<:995
[task 2019-03-29T03:08:36.646Z] 03:08:36 INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:803
[task 2019-03-29T03:08:36.646Z] 03:08:36 INFO - TEST-PASS | browser/base/content/test/plugins/browser_private_clicktoplay.js | Test 2c, Plugin should be activated - true == true -
[task 2019-03-29T03:08:36.647Z] 03:08:36 INFO - Not taking screenshot here: see the one that was previously logged
[task 2019-03-29T03:08:36.647Z] 03:08:36 INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/plugins/browser_private_clicktoplay.js | Uncaught exception - at chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_private_clicktoplay.js:125 - TypeError: popupNotification is null
[task 2019-03-29T03:08:36.648Z] 03:08:36 INFO - Stack trace:
[task 2019-03-29T03:08:36.648Z] 03:08:36 INFO - test2c@chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_private_clicktoplay.js:125:3
[task 2019-03-29T03:08:36.648Z] 03:08:36 INFO - AsyncTester_execTest/<@chrome://mochikit/content/browser-test.js:1106:34
[task 2019-03-29T03:08:36.648Z] 03:08:36 INFO - async
Tester_execTest@chrome://mochikit/content/browser-test.js:1134:12
[task 2019-03-29T03:08:36.649Z] 03:08:36 INFO - nextTest/<@chrome://mochikit/content/browser-test.js:995:14
[task 2019-03-29T03:08:36.649Z] 03:08:36 INFO - SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:803:59
[task 2019-03-29T03:08:36.649Z] 03:08:36 INFO - Leaving test bound test2c
[task 2019-03-29T03:08:36.650Z] 03:08:36 INFO - Entering test bound test3a

Flags: needinfo?(nhnt11)
Assignee

Comment 7

3 months ago

This is a funky failure. I noticed some other failures related to leaving properties behind on windows (because fxmonitor dynamically sets two properties on every window, and since it happens after delayed startup it seems to break that check). To mitigate this, I inlined the PanelUI code into FirefoxMonitor.jsm, and replaced these window properties with Maps in the FirefoxMonitor object that track the same information per-window. This not only seems to fix the properties-left-behind failures but also the clicktoplay test mentioned in the comment above.

Try push is here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e1306ab07e5fa216bea1a81fdb3be69c97813394
https://treeherder.mozilla.org/#/jobs?repo=try&revision=acbf632e288e2a8c29a24095da6ea6cb07d36590
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d2f56f734817f88577372307acf8ae6c19b08c7f

I'll request review on the new patch once the results are in, if it's green.

Flags: needinfo?(nhnt11)
Assignee

Comment 8

3 months ago

Had to also use TestUtils.waitForCondition to reliably wait for the click-to-play notification in private windows in browser_private_clicktoplay.js. This should solve all the issues.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac9cc886ad2728cb3a269117a37764e96b0471d5
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5901e9360a28c8937bd194b81e38a9ccb8fc9df1

Comment 12

2 months ago
Pushed by nhnt11@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/712bd9022cc2
Enable Firefox Monitor by default. r=johannh
https://hg.mozilla.org/integration/autoland/rev/f54fe4e4822f
Inline PanelUI code and avoid leaving behind properties on windows. r=johannh
https://hg.mozilla.org/integration/autoland/rev/1badfbad01fb
Reliably wait for click-to-play popup notification in private windows. r=johannh
https://hg.mozilla.org/integration/autoland/rev/5c20b75a9e32
Ensure we don't set up twice on any window. r=johannh
Assignee

Comment 14

2 months ago

Markus, this bug is trying to enable a system add-on that watches windows, and injects a PopupNotification UI into each one that is displayed upon certain triggers.

It seems like the patches are causing an assertion failure in ClientLayerManager.cpp - https://searchfox.org/mozilla-central/rev/8d78f219702286c873860f39f9ed78bad1a6d062/gfx/layers/client/ClientLayerManager.cpp#84

You added this assertion - could you help me understand exactly what it's trying to catch so I can figure out what's causing it to fail?

Please let me know what questions I can answer to help you understand the situation and also to redirect the ni? if someone else is better equipped to help out with this.

Flags: needinfo?(mstange)

Uh oh, looks like you've found out how to reliably trigger an existing issue.

Matt, could you look into fixing this? Looks like a window's layer manager is being destroyed in a reentrant way, because the call to nsView::DidCompositeWindow somehow causes a stylesheet to be loaded synchronously, which spins a nested event loop... Unfortunately there's no complete stack in the log.

Nihanth, are you using MozAfterPaint listeners in this code somewhere? Are you opening a window from one? If so, you might be able to work around this assertion failure by opening the window asynchronously.

Flags: needinfo?(mstange) → needinfo?(matt.woodrow)
Assignee

Comment 16

2 months ago

(In reply to Markus Stange [:mstange] from comment #15)

Uh oh, looks like you've found out how to reliably trigger an existing issue.

Matt, could you look into fixing this? Looks like a window's layer manager is being destroyed in a reentrant way, because the call to nsView::DidCompositeWindow somehow causes a stylesheet to be loaded synchronously, which spins a nested event loop... Unfortunately there's no complete stack in the log.

Nihanth, are you using MozAfterPaint listeners in this code somewhere? Are you opening a window from one? If so, you might be able to work around this assertion failure by opening the window asynchronously.

I followed up with Markus on IRC and it seems like the issue is that we are manually injecting a stylesheet in a browser-delayed-startup-finished callback, which happens in a MozAfterPaint listener. I'm going to try putting the stylesheet injection (and removal) in Services.tm.dispatchToMainThread() (to avoid spinning a nested event loop) and see if that fixes the issue.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=00e65d52dbfdfbc59e2e7eea3662528c9d38cae9

Flags: needinfo?(nhnt11)
Assignee

Comment 17

2 months ago

This avoids an AssertionError when loading a stylesheet in a nested
event loop. See comment 15 in the bug.

Depends on D26701

Comment 18

2 months ago
Pushed by nhnt11@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/9b0dab8154c0
Enable Firefox Monitor by default. r=johannh
https://hg.mozilla.org/integration/autoland/rev/ae727251b802
Inline PanelUI code and avoid leaving behind properties on windows. r=johannh
https://hg.mozilla.org/integration/autoland/rev/3e538caf5af2
Reliably wait for click-to-play popup notification in private windows. r=johannh
https://hg.mozilla.org/integration/autoland/rev/24d7fb7fd3af
Ensure we don't set up twice on any window. r=johannh
https://hg.mozilla.org/integration/autoland/rev/7e886806ae98
Inject/remove stylesheets in a Services.tm.dispatchToMainThread call. r=johannh

Comment 19

2 months ago

Backed out for causing linux asan leaks

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=success%2Csuperseded%2Ctestfailed%2Cbusted%2Cexception%2Crunnable&searchStr=linux%2Cx64%2Casan%2Cmochitests%2Cwith%2Ce10s%2Ctest-linux64-asan%2Fopt-mochitest-devtools-chrome-e10s-6%2Cm-e10s%28dt6%29&tochange=fccfea2bfef3e02847bf897e7cb25fd7d51a6a8d&fromchange=ca9b52a04b085152ad38b468d3d87e4f56df86f3&selectedJob=239778106

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=239778106&repo=autoland&lineNumber=1769

Backout: https://hg.mozilla.org/integration/autoland/rev/236d2bc601b0604b30e03b2e61a50a3886961edb

[task 2019-04-11T22:18:20.442Z] 22:18:20 INFO - TEST-START | devtools/client/framework/test/browser_browser_toolbox.js

[task 2019-04-11T22:18:40.246Z] 22:18:40 ERROR - GECKO(1396) | ==1606==ERROR: LeakSanitizer: detected memory leaks
[task 2019-04-11T22:18:40.256Z] 22:18:40 INFO - GECKO(1396) | Indirect leak of 32768 byte(s) in 1 object(s) allocated from:
[task 2019-04-11T22:18:40.259Z] 22:18:40 INFO - GECKO(1396) | #0 0x55780d9c5503 in __interceptor_malloc /builds/worker/workspace/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:146:3
[task 2019-04-11T22:18:40.260Z] 22:18:40 INFO - GECKO(1396) | #1 0x7ff8c52c4766 in nsSegmentedBuffer::AppendNewSegment() /builds/worker/workspace/build/src/xpcom/io/nsSegmentedBuffer.cpp:55:22
[task 2019-04-11T22:18:40.261Z] 22:18:40 INFO - GECKO(1396) | #2 0x7ff8c52c3f19 in nsPipe::GetWriteSegment(char*&, unsigned int&) /builds/worker/workspace/build/src/xpcom/io/nsPipe3.cpp:813:25
[task 2019-04-11T22:18:40.262Z] 22:18:40 INFO - GECKO(1396) | #3 0x7ff8c52cd8a2 in nsPipeOutputStream::WriteSegments(nsresult ()(nsIOutputStream, void*, char*, unsigned int, unsigned int, unsigned int*), void*, unsigned int, unsigned int*) /builds/worker/workspace/build/src/xpcom/io/nsPipe3.cpp:1641:17
[task 2019-04-11T22:18:40.263Z] 22:18:40 INFO - GECKO(1396) | #4 0x7ff8c539c091 in NS_InvokeByIndex /builds/worker/workspace/build/src/xpcom/reflect/xptcall/md/unix/xptcinvoke_asm_x86_64_unix.S:106
[task 2019-04-11T22:18:40.264Z] 22:18:40 INFO - GECKO(1396) | #5 0x7ff8c6efa71f in Invoke /builds/worker/workspace/build/src/js/xpconnect/src/XPCWrappedNative.cpp:1624:10
[task 2019-04-11T22:18:40.264Z] 22:18:40 INFO - GECKO(1396) | #6 0x7ff8c6efa71f in Call /builds/worker/workspace/build/src/js/xpconnect/src/XPCWrappedNative.cpp:1171
[task 2019-04-11T22:18:40.265Z] 22:18:40 INFO - GECKO(1396) | #7 0x7ff8c6efa71f in XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) /builds/worker/workspace/build/src/js/xpconnect/src/XPCWrappedNative.cpp:1137
[task 2019-04-11T22:18:40.265Z] 22:18:40 INFO - GECKO(1396) | #8 0x7ff8c6f0082d in XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) /builds/worker/workspace/build/src/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:942:10
[task 2019-04-11T22:18:40.266Z] 22:18:40 INFO - GECKO(1396) | #9 0x7ff8d158b130 in CallJSNative /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:442:13
[task 2019-04-11T22:18:40.267Z] 22:18:40 INFO - GECKO(1396) | #10 0x7ff8d158b130 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:534
[task 2019-04-11T22:18:40.267Z] 22:18:40 INFO - GECKO(1396) | #11 0x7ff8d156c742 in CallFromStack /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:593:10
[task 2019-04-11T22:18:40.268Z] 22:18:40 INFO - GECKO(1396) | #12 0x7ff8d156c742 in Interpret(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:3072
[task 2019-04-11T22:18:40.269Z] 22:18:40 INFO - GECKO(1396) | #13 0x7ff8d1556c08 in js::RunScript(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:422:10
[task 2019-04-11T22:18:40.269Z] 22:18:40 INFO - GECKO(1396) | #14 0x7ff8d158baa3 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:562:13
[task 2019-04-11T22:18:40.269Z] 22:18:40 INFO - GECKO(1396) | #15 0x7ff8d158d722 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:605:8
[task 2019-04-11T22:18:40.270Z] 22:18:40 INFO - GECKO(1396) | #16 0x7ff8d1aa3c9d in js::fun_apply(JSContext*, unsigned int, JS::Value*) /builds/worker/workspace/build/src/js/src/vm/JSFunction.cpp:1183:10
[task 2019-04-11T22:18:40.271Z] 22:18:40 INFO - GECKO(1396) | #17 0x7ff8d158b130 in CallJSNative /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:442:13
[task 2019-04-11T22:18:40.271Z] 22:18:40 INFO - GECKO(1396) | #18 0x7ff8d158b130 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:534
[task 2019-04-11T22:18:40.279Z] 22:18:40 INFO - GECKO(1396) | #19 0x7ff8d156c742 in CallFromStack /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:593:10
[task 2019-04-11T22:18:40.281Z] 22:18:40 INFO - GECKO(1396) | #20 0x7ff8d156c742 in Interpret(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:3072
[task 2019-04-11T22:18:40.282Z] 22:18:40 INFO - GECKO(1396) | #21 0x7ff8d1556c08 in js::RunScript(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:422:10
[task 2019-04-11T22:18:40.283Z] 22:18:40 INFO - GECKO(1396) | #22 0x7ff8d158baa3 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:562:13
[task 2019-04-11T22:18:40.287Z] 22:18:40 INFO - GECKO(1396) | #23 0x7ff8d158d722 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:605:8
[task 2019-04-11T22:18:40.288Z] 22:18:40 INFO - GECKO(1396) | #24 0x7ff8d21cc047 in JS_CallFunctionValue(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/jsapi.cpp:2557:10
[task 2019-04-11T22:18:40.288Z] 22:18:40 INFO - GECKO(1396) | #25 0x7ff8c6ee980a in nsXPCWrappedJS::CallMethod(unsigned short, nsXPTMethodInfo const*, nsXPTCMiniVariant*) /builds/worker/workspace/build/src/js/xpconnect/src/XPCWrappedJSClass.cpp:965:17
[task 2019-04-11T22:18:40.289Z] 22:18:40 INFO - GECKO(1396) | #26 0x7ff8c539d78a in PrepareAndDispatch /builds/worker/workspace/build/src/xpcom/reflect/xptcall/md/unix/xptcstubs_x86_64_linux.cpp:127:37
[task 2019-04-11T22:18:40.290Z] 22:18:40 INFO - GECKO(1396) | #27 0x7ff8c539c66a in SharedStub (/builds/worker/workspace/build/application/firefox/libxul.so+0x229866a)
[task 2019-04-11T22:18:40.291Z] 22:18:40 INFO - GECKO(1396) | #28 0x7ff8c52e1d86 in nsOutputStreamReadyEvent::Run() /builds/worker/workspace/build/src/xpcom/io/nsStreamUtils.cpp:173:20
[task 2019-04-11T22:18:40.292Z] 22:18:40 INFO - GECKO(1396) | #29 0x7ff8c53712b1 in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1180:14
[task 2019-04-11T22:18:40.293Z] 22:18:40 INFO - GECKO(1396) | #30 0x7ff8c53773c8 in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:486:10
[task 2019-04-11T22:18:40.294Z] 22:18:40 INFO - GECKO(1396) | #31 0x7ff8c63b6a1a in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/worker/workspace/build/src/ipc/glue/MessagePump.cpp:88:21
[task 2019-04-11T22:18:40.297Z] 22:18:40 INFO - GECKO(1396) | #32 0x7ff8c62e4392 in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:315:10
[task 2019-04-11T22:18:40.298Z] 22:18:40 INFO - GECKO(1396) | #33 0x7ff8c62e4392 in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:308
[task 2019-04-11T22:18:40.299Z] 22:18:40 INFO - GECKO(1396) | #34 0x7ff8c62e4392 in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:290
[task 2019-04-11T22:18:40.299Z] 22:18:40 INFO - GECKO(1396) | #35 0x7ff8cd714e69 in nsBaseAppShell::Run() /builds/worker/workspace/build/src/widget/nsBaseAppShell.cpp:137:27
[task 2019-04-11T22:18:40.300Z] 22:18:40 INFO - GECKO(1396) | #36 0x7ff8d108def0 in nsAppStartup::Run() /builds/worker/workspace/build/src/toolkit/components/startup/nsAppStartup.cpp:270:30
[task 2019-04-11T22:18:40.301Z] 22:18:40 INFO - GECKO(1396) | #37 0x7ff8d12fca0e in XREMain::XRE_mainRun() /builds/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4572:22

Flags: needinfo?(nhnt11)
Assignee

Comment 20

2 months ago

Oof, I didn't notice this in the previous backout, and I don't know immediately what's going on... I didn't expect this to be so fragile. I'll investigate this with a clear head tomorrow.

Flags: needinfo?(nhnt11)
Assignee

Comment 21

2 months ago

Gijs helped me analyze this on IRC. This leak seems identical to the one being tracked in bug 1444716 comment 7. Seems like the Firefox Monitor code is doing something that changes some timing somewhere that causes things to be, um, less intermittent.

Assignee

Comment 22

2 months ago

On a whim, I refactored the code that sets up the PanelUI to run lazily the first time we actually need to show a notification, rather than adding DOM elements right after delayed startup.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=82698e70167439a16a74eeeba60d0ca2f1956230

Assignee

Comment 23

2 months ago

That didn't do it.

I made patches to merge all the code into api.js (i.e. no more subscripts, no more FirefoxMonitor.jsm), and to punt delayedInit() to right before we call warnIfNeeded - i.e. after we detect a page load.

This avoids doing a lot of I/O (to load subscripts, remote settings, etc) and hopefully stops this code from interfering with tests and triggering memory leaks etc.

Try-all-the-things push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3772ca5d3ae52c8bb3d525bc70dbafbdf02e38a7

Assignee

Comment 24

2 months ago

Seems like just making PanelUI setup be lazy DID do the trick.

Here's a try-all-the-things push just in case: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b44074e15604da31c1b5ce4ec8989daab21c7a1

Attachment #9057343 - Attachment description: Bug 1531838 - Inject/remove stylesheets in a Services.tm.dispatchToMainThread call. r=johannh → Bug 1531838 - Set up fxmonitor PanelUI only when actually showing a notification. r=johannh

Comment 25

2 months ago
Pushed by nhnt11@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/56b4bb2871d1
Enable Firefox Monitor by default. r=johannh
https://hg.mozilla.org/integration/autoland/rev/9f7b63f61c83
Inline PanelUI code and avoid leaving behind properties on windows. r=johannh
https://hg.mozilla.org/integration/autoland/rev/d6e09cefb3c7
Reliably wait for click-to-play popup notification in private windows. r=johannh
https://hg.mozilla.org/integration/autoland/rev/2058918f63f0
Ensure we don't set up twice on any window. r=johannh
https://hg.mozilla.org/integration/autoland/rev/ce0b0350b646
Set up fxmonitor PanelUI only when actually showing a notification. r=johannh
Assignee

Comment 27

2 months ago

Comment on attachment 9051651 [details]
Bug 1531838 - Enable Firefox Monitor by default. r=johannh

Beta/Release Uplift Approval Request

  • Feature/Bug causing the regression: Bug 1525519
  • User impact if declined: User will continue to have Firefox Monitor enabled remotely via Normandy if we don't uplift this.
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: Bug 1525977
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): This change simply enables Firefox Monitor by default, along with a few changes to fix test failures.
    We've already been shipping this feature to the entire release population via Go Faster and Normandy - this simply changes the enabled-state in-tree so we don't have to enable it remotely.

If we don't uplift this, we'll have to continue to use Normandy to enable the feature in 67, which is not recommended by the Normandy team.

  • String changes made/needed: none
Attachment #9051651 - Flags: approval-mozilla-beta?
Assignee

Updated

2 months ago
Attachment #9056886 - Flags: approval-mozilla-beta?
Attachment #9056887 - Flags: approval-mozilla-beta?
Attachment #9056888 - Flags: approval-mozilla-beta?
Attachment #9057343 - Flags: approval-mozilla-beta?
Assignee

Updated

2 months ago
Attachment #9051651 - Flags: approval-mozilla-beta?
Assignee

Updated

2 months ago
Attachment #9056886 - Flags: approval-mozilla-beta?
Assignee

Updated

2 months ago
Attachment #9056887 - Flags: approval-mozilla-beta?
Assignee

Updated

2 months ago
Attachment #9056888 - Flags: approval-mozilla-beta?
Assignee

Updated

2 months ago
Attachment #9057343 - Flags: approval-mozilla-beta?
Assignee

Updated

2 months ago
Regressions: 1544875
Assignee

Updated

2 months ago
Regressions: 1544831

67 will get the addon pref via normandy.

(In reply to Markus Stange [:mstange] from comment #15)

Uh oh, looks like you've found out how to reliably trigger an existing issue.

Matt, could you look into fixing this? Looks like a window's layer manager is being destroyed in a reentrant way, because the call to nsView::DidCompositeWindow somehow causes a stylesheet to be loaded synchronously, which spins a nested event loop... Unfortunately there's no complete stack in the log.

Nihanth, are you using MozAfterPaint listeners in this code somewhere? Are you opening a window from one? If so, you might be able to work around this assertion failure by opening the window asynchronously.

I filed bug 1548189 for this.

Flags: needinfo?(matt.woodrow)
You need to log in before you can comment on or make changes to this bug.