Closed Bug 850730 Opened 9 years ago Closed 9 years ago

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

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 22
Tracking Status
firefox20 --- unaffected
firefox21 + fixed
firefox22 --- fixed

People

(Reporter: bzbarsky, Assigned: fryn)

References

Details

Attachments

(1 file)

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
Attached patch patchSplinter Review
(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"
Assignee: mak77 → fyan
Status: NEW → ASSIGNED
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+
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
Probably worth backporting to aurora too...
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?
https://hg.mozilla.org/mozilla-central/rev/fdc2a6bf577b
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Attachment #724480 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(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.
You need to log in before you can comment on or make changes to this bug.