1460334 - Fix crash caused when attempting to migrate <deck> from a XBL binding to a Custom Element

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, task, P5)

Description

[Brian Grinstead [:bgrins]]

We can follow the pattern of <stringbundle> by making a new general.js file and port the deck impl from general.xml: https://searchfox.org/mozilla-central/source/toolkit/content/widgets/general.xml.

Comment 1

[Brian Grinstead [:bgrins]]

Attachment: Bug 1460334 - Migrate XUL deck from a XBL binding to a Custom Element;

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

Comment 2

[Brian Grinstead [:bgrins]]

Migrating <deck> from XBL to a Custom Element is causing some crashes on browser tests:

Assertion failure: mGlobalObject, at /builds/worker/workspace/build/src/dom/script/ScriptSettings.cpp:157
TEST-UNEXPECTED-FAIL | security/manager/ssl/tests/mochitest/browser/browser_bug627234_perwindowpb.js | application terminated with exit code 11
PROCESS-CRASH | security/manager/ssl/tests/mochitest/browser/browser_bug627234_perwindowpb.js | application crashed [@ mozilla::dom::ScriptSettingsStackEntry::ScriptSettingsStackEntry]

Links:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bcfe71fe311ffaced9c2028f17817cac7a01b33f&selectedJob=177701381
https://treeherder.mozilla.org/logviewer.html#?job_id=177701381&repo=try&lineNumber=11709

Smaug, any ideas on how to fix this? It's reproducible locally with this patch https://hg.mozilla.org/try/rev/58bf7db47d6608682dfd4b1492b747b3f057b903 and `./mach mochitest dom/tests/browser/browser_ConsoleStoragePBTest_perwindowpb.js`

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

I don't understand the stack trace. If we call emplace, we should have a global object, otherwise we would have MOZ_ASSERTed about it.

Comment 4

[Brian Grinstead [:bgrins]]

(In reply to Olli Pettay [:smaug] from comment #3)
> I don't understand the stack trace. If we call emplace, we should have a
> global object, otherwise we would have MOZ_ASSERTed about it.

Huh. It's weird - the binding does very little (even less than <stringbundle> which landed fine).

Looks like if I add a connectedCallback, disconnectedCallback, and constructor then I also get leaks on different suites: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a4ae3fc60d0fd0f23aba0061723dbf0e3c27f0a8&selectedJob=177734288. Maybe that's Bug 1413418 though.

Comment 5

[Brian Grinstead [:bgrins]]

Emilio, is there any chance you can take a look at what's causing the crash in Comment 2? It's reproducible locally as well. This is a pretty trivial Custom Element definition so I think a fix for this will unblock further Custom Element migrations.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

I'll take a look at some point, like tomorrow when it is not holiday in Finland.

Comment 7

[Brian Grinstead [:bgrins]]

(In reply to Olli Pettay [:smaug] from comment #6)
> I'll take a look at some point, like tomorrow when it is not holiday in
> Finland.

Alright, thanks!

Comment 8

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Triage: code cleanup.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Attachment: no_global_for_custom_elements.diff

Since we have the special case for chrome docs below requiring global, better to check we can get global.

remote: 
remote: View your changes here:
remote:   https://hg.mozilla.org/try/rev/680d6198ae7fc3c8638a41d64e0bea6a69d4086f
remote:   https://hg.mozilla.org/try/rev/0f63e6668a23b31bca944b91512ee94697f64f29
remote:   https://hg.mozilla.org/try/rev/62f777387955887103b4d1541ae88302db946bad
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=62f777387955887103b4d1541ae88302db946bad
remote: recorded changegroup in replication log in 0.058s

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8974830 [details] [diff] [review]
no_global_for_custom_elements.diff

bgrins, feel free to test the patch.

Comment 11

[Brian Grinstead [:bgrins]]

Comment on attachment 8974830 [details] [diff] [review]
no_global_for_custom_elements.diff

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

Thanks so much for grabbing this! The test that was crashing locally is now passing with this applied. I have a try push going (+talos for the <deck> migration) to confirm.

Comment 12

[Brian Grinstead [:bgrins]]

I'll make a clone for the actual <deck> migration so we can land the platform fix in this one.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/10fdb852c81a
Fix crash caused when attempting to migrate <deck> from a XBL binding to a Custom Element. r=peterv

Comment 14

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/10fdb852c81a