Closed Bug 1460334 Opened 3 years ago Closed 3 years ago

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

Categories

(Toolkit :: XUL Widgets, task, P5)

task

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: bgrins, Assigned: smaug)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Blocks: war-on-xbl
No longer blocks: 1459245
Depends on: 1459245
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`
Flags: needinfo?(bugs)
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.
Flags: needinfo?(bugs)
(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.
See Also: → 1413418
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.
Flags: needinfo?(emilio)
I'll take a look at some point, like tomorrow when it is not holiday in Finland.
Flags: needinfo?(emilio)
(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!
Triage: code cleanup.
Priority: -- → P5
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
Attachment #8974830 - Flags: review?(peterv)
Assignee: nobody → bugs
Comment on attachment 8974830 [details] [diff] [review]
no_global_for_custom_elements.diff

bgrins, feel free to test the patch.
Attachment #8974830 - Flags: feedback?(bgrinstead)
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.
Attachment #8974830 - Flags: feedback?(bgrinstead) → feedback+
Attachment #8974830 - Flags: review?(peterv) → review+
I'll make a clone for the actual <deck> migration so we can land the platform fix in this one.
Summary: Migrate <deck> from a XBL binding to a Custom Element → Fix crash caused when attempting to migrate <deck> from a XBL binding to a Custom Element
Blocks: 1461388
Attachment #8974439 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Keywords: checkin-needed
No longer depends on: 1459245
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/10fdb852c81a
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.