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

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Boris Zbarsky [:bzbarsky]]

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.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

We should just exclude about:blank (mea culpa: bug 790934 comment 15).

Comment 2

[Marco Bonardo [:mak]]

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.

Comment 3

[Frank Yan (:fryn)]

Attachment: 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")

Comment 4

[Marco Bonardo [:mak]]

/^about:(?!blank$)/i should be enough as a regex.. though it's probably more readable a simple doc.documentURI.toLowerCase() != "about:blank"

Comment 5

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 6

[Frank Yan (:fryn)]

Thank you for the quick review, Gavin. :)

https://hg.mozilla.org/integration/mozilla-inbound/rev/fdc2a6bf577b

Comment 7

[Boris Zbarsky [:bzbarsky]]

Probably worth backporting to aurora too...

Comment 8

[Frank Yan (:fryn)]

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

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/fdc2a6bf577b

Comment 10

[Frank Yan (:fryn)]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e9b8ca7890a7