1281670 - Style attribute is not parsed into style object under certain conditions

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[neil]

Attachment: 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.

Comment 1

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Hi Edgar, could you please take a look? Thanks!

Comment 2

[Edgar Chen [:edgar]]

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

Comment 3

[Edgar Chen [:edgar]]

(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');

Comment 4

[Boris Zbarsky [:bzbarsky]]

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

Comment 5

[Edgar Chen [:edgar]]

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

Comment 6

[Boris Zbarsky [:bzbarsky]]

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

Comment 7

[Edgar Chen [:edgar]]

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

Comment 8

[Edgar Chen [:edgar]]

Attachment: Patch, v1

Comment 9

[Edgar Chen [:edgar]]

Attachment: Patch, v2

Comment 10

[Edgar Chen [:edgar]]

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.

Comment 11

[Edgar Chen [:edgar]]

Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fed8d69bf6cc&filter-tier=1&group_state=expanded

Comment 12

[Boris Zbarsky [:bzbarsky]]

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.

Comment 13

[Edgar Chen [:edgar]]

Attachment: Patch, v3, r=bz

Address review comment #12.

Comment 14

[Edgar Chen [:edgar]]

Attachment: Add web-platform-test, v1

Comment 15

[Boris Zbarsky [:bzbarsky]]

The web platform test  needs a title.  And do you still need the mochitest?

Comment 16

[Edgar Chen [:edgar]]

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

Comment 17

[Edgar Chen [:edgar]]

Attachment: 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.

Comment 18

[Boris Zbarsky [: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

Comment 19

[Edgar Chen [:edgar]]

Attachment: Patch, v5, r=bz

Address review comment #18.

Comment 20

[Edgar Chen [:edgar]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b11c5d7c87032c698835b4a718cd9a05419a0fc8&filter-tier=1&group_state=expanded

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff1148ab6819
Keep reparsing style attribute if we had parsed it once. r=bz

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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