Closed Bug 1546338 Opened 5 years ago Closed 5 years ago

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

Categories

(Thunderbird :: General, task)

Product:
Thunderbird
Component:
General
Type:
task
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 68.0

People

(Reporter: mkmelin, Assigned: mkmelin)

References

Details

Attachments

(3 files, 1 obsolete file)

bug1546338_createElement.patch
169.05 KB, patch
jorgk-bmo
: review+
Details | Diff | Splinter Review
bug1546338_createElement_part2.patch
17.94 KB, patch
jorgk-bmo
: review+
Details | Diff | Splinter Review
2b1b73b8c5d7 - take 2
5.77 KB, application/octet-stream
jorgk-bmo
: approval-comm-beta+
Details

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?

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]
Depends on: 1551320

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.

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

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

Attachment #9065490 - Flags: review?(jorgk)
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+

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: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 68.0

(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.

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)
Attachment #9065912 - Flags: review?(jorgk) → review+
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

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

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

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.

Flags: needinfo?(mkmelin+mozilla)
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

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

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

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)

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

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

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

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.

20190524104146 still crashes

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.

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
Attached 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.

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.

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
Attached file 2b1b73b8c5d7 - take 2 —

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.

Attachment

General

Creator:
Created:
Updated:
Size: