Port [Bug 1551320 - In XUL documents replace all callers of createElement with createXULElement]

RESOLVED FIXED in Thunderbird 68.0

Status

task
RESOLVED FIXED
2 months ago
21 days ago

People

(Reporter: mkmelin, Assigned: mkmelin)

Tracking

Trunk
Thunderbird 68.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Assignee

Description

2 months ago

We should port bug 1485426 so that we make sure to create the right type of elements - otherwise there will be problems when using e.g. document.createElement (expecting a xul element) in an HTML document.

Per bug 1485426 comment 2:
"
For future reference to find more createElements from XUL:

  1. Apply the patch https://hg.mozilla.org/try/rev/e15fbc016dfdb0da4862f0a7415154bfa338c023
  2. use try fuzzy with "instrumentation"
  3. Download all logs in folder
  4. To get a list of all the unique callers run
    cat .log | grep "CreateElement called" | sed -E 's/.|/LOG/g' | sort | uniq > caller.txt
    or
    to get filenames only:
    cat .log | grep "CreateElement called" | sed -E 's/.|/LOG/g' | sed -E 's/."(.)".*/\1/g' | sort | uniq > file_only.txt

The above commands could be simplified, but it was built as I went.
"

Geoff, want to take this on at some point?

Assignee

Comment 1

Last month

We need to do this before bug 1551320, or we're all busted.

Assignee: nobody → mkmelin+mozilla
Summary: Port [Bug 1485426 - Audit callers of document.createElement in XUL documents] to Thunderbird → Port [Bug 1551320 - In XUL documents replace all callers of createElement with createXULElement]
Assignee

Updated

Last month
Depends on: 1551320
Assignee

Comment 2

Last month

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=16a984c31fd77170c25a68accbaf9a45ecf85999

If it's green, it can land at any time to prevent bustage once bug 1551320 lands.
I think this patch is still not 100% complete.

Status: NEW → ASSIGNED

Isn't bug 1551320 planned to land after the merge? If yes, we shouldn't land it before.

Assignee

Comment 4

Last month

I'm assuming yes, but not sure.
Pros and cons: this is changing a lot of callers to be correct. If we don't land before merge, backporting other changes might be a lot harder later

Comment 5

Last month

You're going to attach the patch here and have it reviewed? I'm not getting it off try for landing, right ;-)

Assignee

Comment 6

Last month
Attachment #9065490 - Flags: review?(jorgk)

Comment 7

Last month
Comment on attachment 9065490 [details] [diff] [review]
bug1546338_createElement.patch

Hmm, so this is more "correct" according to "changing a lot of callers to be correct"?
Attachment #9065490 - Flags: review?(jorgk) → review+

Comment 8

Last month

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/7e3a1808d30d
Port [Bug 1551320 - In XUL documents replace all callers of createElement with createXULElement]. r=jorgk

Status: ASSIGNED → RESOLVED
Closed: Last month
Resolution: --- → FIXED

Updated

Last month
Target Milestone: --- → Thunderbird 68.0
Assignee

Comment 9

Last month

(In reply to Jorg K (GMT+2) from comment #7)

Comment on attachment 9065490 [details] [diff] [review]
bug1546338_createElement.patch

Hmm, so this is more "correct" according to "changing a lot of callers to be
correct"?

createElement would create an element in the current document namespace. So it's not "wrong" earlier, but not as explicit.

Assignee

Comment 10

Last month

Some more instances (a few snuck in in landings, and the ones in xml files I didn't do earlier.)

Attachment #9065912 - Flags: review?(jorgk)

Updated

Last month
Attachment #9065912 - Flags: review?(jorgk) → review+

Comment 12

Last month
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/6b97e6f65af8
fix some more instances of Port [Bug 1551320 - In XUL documents replace all callers of createElement with createXULElement]. r=jorgk

Comment 13

27 days ago

After landing of bug 1551320, we crashed pretty terribly. I found at least two more that needed fixing. Landing now.

Comment 14

27 days ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/2b1b73b8c5d7
Follow-up: fix some more instances. rs=bustage-fix

Comment 15

27 days ago

Magnus, can you please check these. "label" is a XUL element, but "div" isn't. However, I got the horrible error traceback for that "div" around line 140.

Updated

27 days ago
Flags: needinfo?(mkmelin+mozilla)

Comment 16

27 days ago
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/886769faf4ff
Fix two instances of createElement that should be createXULElement; rs=bustage-fix

Comment 17

27 days ago

There's another one missing that makes Mozmill composition/test-drafts.js fail. I'll push a fix now.

Comment 18

27 days ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/eebe1c3a65ec
Fix one instance of createElement that should be createXULElement. rs=bustage-fix
Assignee

Comment 19

27 days ago

The html elements needs to be created with createElementNS.
Working on correcting these now, please don't land any more "wrong" changes.

Flags: needinfo?(mkmelin+mozilla)

Comment 20

27 days ago

OK, however, the "wrong" changes worked ;-) - We had Mozmill totally busted last night. I'll leave it to the assignee.

Assignee

Comment 21

27 days ago

Sure, work as in not crash due to the assertion. But it wouldn't actually do anything useful. A <xul:b> tag doesn't do anything.

Quick try run with the corrections now: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=8ef1a5643809680fdb4445ad8b51991b66dc0f9a

Comment 22

27 days ago
Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/a315856a66fa
fix some instances of createElement that were creating HTML elements to use in a xul document. rs=bustage-fix

Comment 23

27 days ago

On updating to Daily 20190524080013 on Linux x86_64 this morning I started experiencing a start up crash MOZ_RELEASE_ASSERT(false) (CreateElement() not allowed in XUL document.) which prevents use of Thunderbird. Downloading the Fri, May 24, 03:41:46 build to check on that.

Comment 24

27 days ago

20190524104146 still crashes

Comment 26

27 days ago

mkmelin suggested safe mode and it no longer crashed. I uninstalled gdata-provider.en-US.xpi and was able to start up. I installed the latest version and was also able to start.

Comment 27

27 days ago
Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/69f918337942
re-adjust event iframe to be xul:iframe, to fix testEventDialogSize.js. rs=bustage-fix

Comment 28

25 days ago
Posted patch 2b1b73b8c5d7 (obsolete) — Splinter Review

Here are the five patches folded that landed on TB 69. We might as will have that stuff join its friends.

Attachment #9067576 - Flags: approval-comm-beta+

The iframe in applications.js is also supposed to be a XUL iframe, not an HTML iframe.

Comment 30

24 days ago

I was wondering that, so I tested it. That code is run when looking at file link providers. It looked to me like it was working, but now I see that the picture is cut off. I'll try the XUL way now.
EDIT: Yes, xul:iframe certainly working better.

Comment 31

24 days ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/166ea12f3b4b
Follow-up: re-adjust cloud file account info iframe to be xul:iframe. r=me

Comment 32

24 days ago

Adjusting beta patch accordingly, thanks Geoff!

Attachment #9067576 - Attachment is obsolete: true
Attachment #9067666 - Flags: approval-comm-beta+
You need to log in before you can comment on or make changes to this bug.