Ryan VanderMeulen [:RyanVM] Reporter
	Description • 9 years ago

[Tracking Requested - why for this release]: Failures across multiple test suites when Gecko 39 is merged to Aurora.

The uplift is Monday. This needs to be fixed (or bug 1094821 backed out) ASAP.

xpcshell:
https://treeherder.mozilla.org/logviewer.html#?job_id=6020223&repo=try

16:00:03 WARNING - TEST-UNEXPECTED-FAIL | services/sync/tests/unit/test_prefs_store.js | xpcshell return code: 0 


mochitest-bc:
https://treeherder.mozilla.org/logviewer.html#?job_id=6020221&repo=try
https://treeherder.mozilla.org/logviewer.html#?job_id=6020223&repo=try

Lots, sorry.


mochitest-other:
https://treeherder.mozilla.org/logviewer.html#?job_id=6018620&repo=try

15:27:24 INFO - 2649 INFO TEST-START | toolkit/mozapps/update/tests/chrome/test_0011_check_basic.xul
15:27:25 INFO - ++DOMWINDOW == 229 (0x7fca32809c00) [pid = 1930] [serial = 5592] [outer = 0x7fca65b0b400]
15:27:25 INFO - 1427495245641 addons.xpi WARN Attempting to activate an already active default theme
15:27:25 INFO - [Parent 1930] ###!!! ASSERTION: stylesheet not found: 'mInUnlinkOrDeletion', file /builds/slave/try-l64-d-00000000000000000000/build/src/dom/base/nsDocument.cpp, line 4317
15:27:25 INFO - #01: nsStyleLinkElement::DoUpdateStyleSheet(nsIDocument*, mozilla::dom::ShadowRoot*, nsICSSLoaderObserver*, bool*, bool*, bool) [dom/base/nsStyleLinkElement.cpp:334]
15:27:25 INFO - #02: nsStyleLinkElement::UpdateStyleSheetInternal(nsIDocument*, mozilla::dom::ShadowRoot*, bool) [dom/base/nsStyleLinkElement.cpp:224]
15:27:25 INFO - #03: mozilla::dom::XMLStylesheetProcessingInstruction::UnbindFromTree(bool, bool) [dom/xml/XMLStylesheetProcessingInstruction.cpp:76]
15:27:25 INFO - #04: nsINode::doRemoveChildAt(unsigned int, bool, nsIContent*, nsAttrAndChildArray&) [dom/base/nsINode.cpp:1648]
15:27:25 INFO - #05: nsDocument::RemoveChildAt(unsigned int, bool) [dom/base/nsDocument.cpp:4170]
15:27:25 INFO - #06: nsINode::Remove() [dom/base/nsINode.cpp:1594]
15:27:25 INFO - #07: mozilla::dom::CharacterDataBinding::remove [obj-firefox/dom/bindings/CharacterDataBinding.cpp:321]
15:27:25 INFO - #08: mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) [dom/bindings/BindingUtils.cpp:2494]
15:27:25 INFO - #09: js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) [js/src/jscntxtinlines.h:236]
15:27:25 INFO - #10: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) [js/src/vm/Interpreter.cpp:502]
15:27:25 INFO - #11: Interpret [js/src/vm/Interpreter.cpp:2617]
15:27:25 INFO - #12: js::RunScript(JSContext*, js::RunState&) [js/src/vm/Interpreter.cpp:452]
15:27:25 INFO - #13: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) [js/src/vm/Interpreter.cpp:521]
15:27:25 INFO - #14: js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) [js/src/vm/Interpreter.cpp:558]
15:27:25 INFO - #15: JS_CallFunctionValue(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) [js/src/jsapi.cpp:4325]
15:27:25 INFO - #16: nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS*, unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) [js/xpconnect/src/XPCWrappedJSClass.cpp:1210]
15:27:25 INFO - #17: PrepareAndDispatch [xpcom/reflect/xptcall/md/unix/xptcstubs_x86_64_linux.cpp:124]
15:27:25 INFO - #18: NS_InvokeByIndex [xpcom/reflect/xptcall/md/unix/xptcinvoke_x86_64_unix.cpp:178]

	Brian Grinstead [:bgrins] Assignee
	Comment 1 • 9 years ago

I'm looking into this.  I believe the errors are due to the lightweightThemes.selectedThemeID pref which is set only in dev edition [0].

I think the simplest way to solve this would be to simply reset this pref for the test suites (we have separate test coverage for the selected theme feature).  Unfortunately simply resetting that to an empty string in testing/profiles/prefs_general.js doesn't seem to be changing the default value defined in firefox.js, so I'm going to keep looking. What would really help is a way for the test harnesses to remove a pref that was set in firefox.js entirely.

[0]: https://dxr.mozilla.org/mozilla-central/source/browser/app/profile/firefox.js#1331

	Brian Grinstead [:bgrins] Assignee
	Comment 2 • 9 years ago

Just in case, here is a try push with a backout of Bug 1094821 part 3: https://treeherder.mozilla.org/#/jobs?repo=try&revision=06ce0543d918.  I believe there shouldn't be any test failures from the first 2 parts of that bug since the only devedition specific bits were in this third part.  We may want to keep the change to browser.properties though since we will need that string when relanding.

	Brian Grinstead [:bgrins] Assignee
	Comment 3 • 9 years ago

Here is a patch that backs out the part 3 changes from Bug 1094821, and a try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c4631454bd5.  There is still a lot of orange but they *seem* unrelated and I'm not sure if there are other issues with that Aurora simulation.

	Ryan VanderMeulen [:RyanVM] Reporter
	Comment 4 • 9 years ago

The rest of the orange looks like known issues. Thanks!

	Brian Grinstead [:bgrins] Assignee
	Comment 5 • 9 years ago

OK, I've been messing around with the test harness to allow us to remove the pref in devedition, but it will need further discussion and reviews, so the safest bet will be to back this final part out for 39 and then ship it in 40 once I have a chance to get that done.

I have a pending try push against fxteam with the backout applied to make sure it's OK to proceed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=46e70c6dceae.

It probably makes the most sense to keep 1094821 resolved since it has changes to the LW Theme manager that can affect addon-compat, and then file a new bug to reland this part with any needed test fixes.

	Brian Grinstead [:bgrins] Assignee
	Comment 6 • 9 years ago

Comment on attachment 8585231 [details] [diff] [review]
backout-lwtheme-devedition-changes.patch

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

::: browser/components/nsBrowserGlue.js
@@ +1962,5 @@
>        }
>      }
>  
>      if (currentUIVersion < 28) {
> +      // Placeholder for 01482cdccd72 which was backed out (Bug 1094821)

I kept this migration block in place for now.  We will need to bump to the next UI version when relanding in Bug 1148996

	Ryan VanderMeulen [:RyanVM] Reporter
	Comment 7 • 9 years ago

FYI, we're going to need to land this directly on m-c at this point. It's officially blocking the m-c -> Aurora merge. Need a review here ASAP.

	Brian Grinstead [:bgrins] Assignee
	Comment 8 • 9 years ago

Comment on attachment 8585231 [details] [diff] [review]
backout-lwtheme-devedition-changes.patch

Gijs, does this sound like a reasonable plan?  See comment 5 (and the change to nsBrowserGlue in this backout patch).

	Sylvestre Ledru [:Sylvestre]
	Comment 9 • 9 years ago

Tracking as it is critical for the merge and the gtb of 38 beta 1.

	Brian Grinstead [:bgrins] Assignee
	Comment 10 • 9 years ago

Removed the placeholder if block from nsBrowserGlue after discussing with Gavin

	Brian Grinstead [:bgrins] Assignee
	Comment 11 • 9 years ago

Landed the backout on m-c: https://hg.mozilla.org/mozilla-central/rev/1b6bf6612c0f.

	:Gijs (he/him)
	Comment 12 • 9 years ago

(sorry for the slowness here, I'm in transit and a well-laid travel plan that would have had me home a few hours ago has been upset by weather, and so I'm still on Munich's airport wifi and not as responsive as I would otherwise have been...)


Retrospective r+ on the backout, though I would like to re-verify how the original works with trypushes that toggle the devedition flag, and then reland and uplift. That can happen in the other bug, though.

	Ryan VanderMeulen [:RyanVM] Reporter
	Comment 13 • 9 years ago

Turns out this bug was obscuring another permafail on that Try push (bug 1149329) :(. Oh well, at least this one is fixed.