Style attribute is not parsed into style object under certain conditions

RESOLVED FIXED in Firefox 51

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: neil, Assigned: edgar)

Tracking

47 Branch
mozilla51
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

2 years ago
Created attachment 8764443 [details]
testcase.html

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/51.0.2704.103 Safari/537.36

Steps to reproduce:

When using the DOMParser to parse and sanitise a document outside of the "live" document in the page:
1. Read properties on the style object of an element
2. Remove the style property of the element
3. Add back a new style property to the element
4. Try to read a property on the style object of the element


Actual results:

The style object is empty (the style attribute has not been parsed). Worse, if you modify a property on the style object, it overwrites the whole style attribute currently set.


Expected results:

The style object contains a parsed version of the attribute. Setting a property on the style object does not modify independent style properties.

Minimal test case attached. Tested in latest stable release (Firefox 47.0).

We use this access pattern as part of our rendering process for emails at https://www.fastmail.com and this bug is resulting in Firefox users seeing degraded fidelity in rendering emails compared to users of other browsers.

Updated

2 years ago
Component: Untriaged → DOM
Product: Firefox → Core
Hi Edgar, could you please take a look? Thanks!
Flags: needinfo?(echen)
(Assignee)

Updated

2 years ago
Assignee: nobody → echen
Flags: needinfo?(echen)
(Assignee)

Comment 2

2 years ago
I did some tests, 

1. If you modify a property on the style object without removing and calling "setAttribute",
   it works good.
   e.g. http://software.hixie.ch/utilities/js/live-dom-viewer/saved/4329

2. If you remove attribute and add back by modifying the property on the style object
   (instead of calling "setAttribute"), it works good.
   e.g. http://software.hixie.ch/utilities/js/live-dom-viewer/saved/4330

3. If you call "setAttribute" (even doesn't remove attribute first), issue happens.
   e.g. http://software.hixie.ch/utilities/js/live-dom-viewer/saved/4332

It looks like the issue is related to "setAttribute".
(Assignee)

Comment 3

2 years ago
(In reply to Edgar Chen [:edgar][:echen] from comment #2)
> It looks like the issue is related to "setAttribute".

Another clue: issue happens only when the style object has been accessed.

e.g.

works good:
  var doc = (new DOMParser()).parseFromString('<div style="color:red">Foo</div>', 'text/html');
  var div = doc.querySelector('div');
  div.setAttribute('style', 'color:green');

issue happens:
  var doc = (new DOMParser()).parseFromString('<div style="color:red">Foo</div>', 'text/html');
  var div = doc.querySelector('div');
  var foo = div.style; // just access style object, do nothing
  div.setAttribute('style', 'color:green');
(Assignee)

Updated

2 years ago
See Also: → bug 418214
Hmm.  Since we do in fact keep around the string value nowadays, it might just be possible to back the patch for bug 418214 out entirely...
(Assignee)

Comment 5

2 years ago
(In reply to Boris Zbarsky [:bz] from comment #4)
> Hmm.  Since we do in fact keep around the string value nowadays, it might
> just be possible to back the patch for bug 418214 out entirely...

Thanks for the comment. I will try it.
On the other hand, we'd probably not want to parse it on initial attr set, for performance reasons.  We'd just want to keep reparsing it once we'd parsed it once...
(Assignee)

Comment 7

2 years ago
(In reply to Boris Zbarsky [:bz] from comment #6)
> On the other hand, we'd probably not want to parse it on initial attr set,
> for performance reasons.  We'd just want to keep reparsing it once we'd
> parsed it once...

Got it.
(Assignee)

Comment 8

2 years ago
Created attachment 8775920 [details] [diff] [review]
Patch, v1
(Assignee)

Comment 9

2 years ago
Created attachment 8776433 [details] [diff] [review]
Patch, v2
Attachment #8775920 - Attachment is obsolete: true
(Assignee)

Comment 10

2 years ago
Comment on attachment 8776433 [details] [diff] [review]
Patch, v2

Review of attachment 8776433 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Boris Zbarsky [:bz] from comment #6)
> On the other hand, we'd probably not want to parse it on initial attr set,
> for performance reasons.  We'd just want to keep reparsing it once we'd
> parsed it once...

I keep the changes of bug 418214 since we don't want to parse style on initial attr set.
But add a condition in ParseStyleAttribute() to keep reparsing style once we had parsed it.

Hi Boris, may I have your review? Thank you.
Attachment #8776433 - Flags: review?(bzbarsky)
(Assignee)

Updated

2 years ago
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment on attachment 8776433 [details] [diff] [review]
Patch, v2

>+      DOMSlots()->mStyle ||

This will force slots allocation, which is not needed here.  What you want is to add a helper that calls GetExistingDOMSlots() and if that returns non-null returns its mStyle, otherwise returns null.  Or just returns a boolean, really...

The test should probably be a web platform test, since this is all covered by specs.

r=me with those fixed.
Attachment #8776433 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 13

2 years ago
Created attachment 8778755 [details] [diff] [review]
Patch, v3, r=bz

Address review comment #12.
Attachment #8776433 - Attachment is obsolete: true
Attachment #8778755 - Flags: review+
(Assignee)

Comment 14

2 years ago
Created attachment 8778756 [details] [diff] [review]
Add web-platform-test, v1
The web platform test  needs a title.  And do you still need the mochitest?
(Assignee)

Comment 16

2 years ago
(In reply to Boris Zbarsky [:bz] from comment #15)
> The web platform test  needs a title.  And do you still need the mochitest?

Thanks for reminding me. No, we don't need the mochitest.
(Assignee)

Comment 17

2 years ago
Created attachment 8779207 [details] [diff] [review]
Patch, v4

Address comment#15,
1) squash patches
2) add title in wpt 
3) remove mochitest test

Hi Boris, may I have your review again, in case I missed something?
Thank you.
Attachment #8778755 - Attachment is obsolete: true
Attachment #8778756 - Attachment is obsolete: true
Attachment #8779207 - Flags: review?(bzbarsky)
Comment on attachment 8779207 [details] [diff] [review]
Patch, v4

>+    assert_equals(div.getAttribute("style"), 'color: red',
>+                  'Shouldn not affect style attribute');
>+}, 'Parsing of initial style attribute');

"Should not affect"?  Same elsewhere.  And it might be better to label this assert_equals as "Value of style attribute should match the string value that was set" or something.

r=me
Attachment #8779207 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 19

2 years ago
Created attachment 8779651 [details] [diff] [review]
Patch, v5, r=bz

Address review comment #18.
Attachment #8764443 - Attachment is obsolete: true
Attachment #8779207 - Attachment is obsolete: true
Attachment #8779651 - Flags: review+

Comment 21

2 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff1148ab6819
Keep reparsing style attribute if we had parsed it once. r=bz
Keywords: checkin-needed

Comment 22

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ff1148ab6819
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.