All users were logged out of Bugzilla on October 13th, 2018

Firefox UI sets random attributes on root nodes, leads to spec violations

RESOLVED FIXED in Firefox 21

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: bzbarsky, Assigned: fryn)

Tracking

Trunk
Firefox 22
Points:
---

Firefox Tracking Flags

(firefox20 unaffected, firefox21+ fixed, firefox22 fixed)

Details

Attachments

(1 attachment)

Bug 790934 added a spec violation that's web-observable.  Specifically, it's changing the DOM for about:blank documents, which is web-visible and changing it is a spec violation.
We should just exclude about:blank (mea culpa: bug 790934 comment 15).
I was not aware of this about:blank specification, my assumption to exclude about:blank was for optimization reasons.
Many thanks for pointing it out.
I will add the interdiff to exclude about:blank here, hopefully by end of today.
Assignee: nobody → mak77

Updated

6 years ago
status-firefox20: --- → unaffected
status-firefox21: --- → affected
tracking-firefox21: --- → ?
(Assignee)

Comment 3

6 years ago
Created attachment 724480 [details] [diff] [review]
patch

(Sorry for the collision, Marco. I wrote this before you assigned it to yourself.)

We need to exclude everything of the pattern:

/^about:blank($|[#?])/i

If we want to avoid using a RegExp, we can just approximate it with the following, which is what this patch does:

doc.documentURI.startsWith("about:") &&
!doc.documentURI.toLowerCase().startsWith("about:blank")
Attachment #724480 - Flags: review?(gavin.sharp)
/^about:(?!blank$)/i should be enough as a regex.. though it's probably more readable a simple doc.documentURI.toLowerCase() != "about:blank"

Updated

6 years ago
Assignee: mak77 → fyan
Status: NEW → ASSIGNED

Updated

6 years ago
tracking-firefox21: ? → +
Comment on attachment 724480 [details] [diff] [review]
patch

Given that it's quite unlikely for about:blankFOO to exist, and if it did we wouldn't care about not adding the listeners to it, this seems fine.
Attachment #724480 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 6

6 years ago
Thank you for the quick review, Gavin. :)

https://hg.mozilla.org/integration/mozilla-inbound/rev/fdc2a6bf577b
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: --- → Firefox 22
Version: unspecified → Trunk
(Reporter)

Comment 7

6 years ago
Probably worth backporting to aurora too...
(Assignee)

Comment 8

6 years ago
Comment on attachment 724480 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by: the landing of bug 90934
User impact if declined: Firefox would continue changing the DOM of about:blank pages in a way that violates a web spec and is visible to any web page that opens about:blank pages
Testing completed: locally and just landed on mozilla-inbound
Risk to taking this patch: minimal; super localized fix
String or UUID changes made by this patch: none
Attachment #724480 - Flags: approval-mozilla-aurora?

Comment 9

6 years ago
https://hg.mozilla.org/mozilla-central/rev/fdc2a6bf577b
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Attachment #724480 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 10

6 years ago
(In reply to Marco Bonardo [:mak] from comment #4)
> /^about:(?!blank$)/i should be enough as a regex..

No, that's not enough, because any web page can open URIs like about:BLAnk?hi that are still interpreted to be the same document as about:blank, and we should not, from our TabsProgressListener, modify the DOM of any document that web pages can create and read.
https://hg.mozilla.org/releases/mozilla-aurora/rev/e9b8ca7890a7
status-firefox21: affected → fixed
status-firefox22: --- → fixed
You need to log in before you can comment on or make changes to this bug.