ASSERTION FAILED: factor > 0

ASSERTION FAILED: factor > 0

If images there are inside templates, we shouldn't parse their sizes, just when the document is active.

BUG=423279
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=184070

[bzsolt, 2014-10-14 12:25:23]

zsborbely.u-szeged@partner.samsung.com changed reviewers:
+ ojan@chromium.org, yoav@yoav.ws

[Yoav Weiss, 2014-10-14 15:21:12]

On 2014/10/14 12:25:23, zsborbely.u-szeged wrote:

Thanks for finding this bug! :)

If I understand the scenario that you've hit:
* SizesAttributeParser was created for a document with no frame. (in your test
case: an HTMLImageElement inside a template)
* The sizes value included a value that depends on the viewport width/height,
which with no frame present, is 0.
* The resulting factor was zero.

Is that correct?

If so, I'm not sure special casing it and avoiding the length calculation
altogether in this case is the right way to go.
I'm also not sure the desired behavior for an image inside a template should be
the loading of the resource with a 0 sized viewport.

[ojan, 2014-10-14 22:22:30]

ojan@chromium.org changed reviewers:
+ adamk@chromium.org, cbiesinger@chromium.org
- ojan@chromium.org

[ojan, 2014-10-14 22:22:31]

Images inside templates don't load at all, so we shouldn't be hitting the size
parsing code at all, right?

[adamk, 2014-10-14 22:43:08]

https://codereview.chromium.org/653023002/diff/1/Source/core/html/HTMLImageEl...
File Source/core/html/HTMLImageElement.cpp (right):

https://codereview.chromium.org/653023002/diff/1/Source/core/html/HTMLImageEl...
Source/core/html/HTMLImageElement.cpp:309: bool hasLocalFrame =
document().frame() ? true : false;
To answer Ojan's question, this is exactly the reason images "don't load" in
templates.

Ojan, are you asking for this check to move further up?

[ojan, 2014-10-14 22:46:08]

ojan@chromium.org changed reviewers:
+ ojan@chromium.org

[ojan, 2014-10-14 22:46:09]

https://codereview.chromium.org/653023002/diff/1/Source/core/html/HTMLImageEl...
File Source/core/html/HTMLImageElement.cpp (right):

https://codereview.chromium.org/653023002/diff/1/Source/core/html/HTMLImageEl...
Source/core/html/HTMLImageElement.cpp:309: bool hasLocalFrame =
document().frame() ? true : false;
On 2014/10/14 22:43:07, adamk wrote:
> To answer Ojan's question, this is exactly the reason images "don't load" in
> templates.
> 
> Ojan, are you asking for this check to move further up?

I don't know the code here well enough to know what the right thing to do is,
but at first blush, it seems to me like we shouldn't parse sizes on images in
templates. We'll reparse when we stamp out the new image, right?

[adamk, 2014-10-14 22:48:22]

https://codereview.chromium.org/653023002/diff/1/Source/core/html/HTMLImageEl...
File Source/core/html/HTMLImageElement.cpp (right):

https://codereview.chromium.org/653023002/diff/1/Source/core/html/HTMLImageEl...
Source/core/html/HTMLImageElement.cpp:309: bool hasLocalFrame =
document().frame() ? true : false;
On 2014/10/14 22:46:09, ojan-only-code-yellow-reviews wrote:
> On 2014/10/14 22:43:07, adamk wrote:
> > To answer Ojan's question, this is exactly the reason images "don't load" in
> > templates.
> > 
> > Ojan, are you asking for this check to move further up?
> 
> I don't know the code here well enough to know what the right thing to do is,
> but at first blush, it seems to me like we shouldn't parse sizes on images in
> templates. We'll reparse when we stamp out the new image, right?

Sounds like we should reparse when we switch documents (didMoveToNewDocument).

[Yoav Weiss, 2014-10-15 03:54:34]

On 2014/10/14 22:43:08, adamk wrote:
>
https://codereview.chromium.org/653023002/diff/1/Source/core/html/HTMLImageEl...
> File Source/core/html/HTMLImageElement.cpp (right):
> 
>
https://codereview.chromium.org/653023002/diff/1/Source/core/html/HTMLImageEl...
> Source/core/html/HTMLImageElement.cpp:309: bool hasLocalFrame =
> document().frame() ? true : false;
> To answer Ojan's question, this is exactly the reason images "don't load" in
> templates.
> 

Looking further into it, I believe the reason images don't currently load in
templates is that ImageLoader::doUpdateFromElement has a check for
document().isActive(), and returns when it is false.

[Yoav Weiss, 2014-10-15 04:04:48]

On 2014/10/14 22:46:09, ojan-only-code-yellow-reviews wrote:
>
https://codereview.chromium.org/653023002/diff/1/Source/core/html/HTMLImageEl...
> File Source/core/html/HTMLImageElement.cpp (right):
> 
>
https://codereview.chromium.org/653023002/diff/1/Source/core/html/HTMLImageEl...
> Source/core/html/HTMLImageElement.cpp:309: bool hasLocalFrame =
> document().frame() ? true : false;
> On 2014/10/14 22:43:07, adamk wrote:
> > To answer Ojan's question, this is exactly the reason images "don't load" in
> > templates.
> > 
> > Ojan, are you asking for this check to move further up?
> 
> I don't know the code here well enough to know what the right thing to do is,
> but at first blush, it seems to me like we shouldn't parse sizes on images in
> templates. We'll reparse when we stamp out the new image, right?

I agree. I think we should probably add an isActive check to
HTMLImageElement::selectSourceURL and to insertedInto.
(As an aside: Looks like this code needs some tidying up, so that we can run the
isActive check in a single place)

[Yoav Weiss, 2014-10-15 04:10:00]

On 2014/10/14 22:48:22, adamk wrote:
>
https://codereview.chromium.org/653023002/diff/1/Source/core/html/HTMLImageEl...
> File Source/core/html/HTMLImageElement.cpp (right):
> 
>
https://codereview.chromium.org/653023002/diff/1/Source/core/html/HTMLImageEl...
> Source/core/html/HTMLImageElement.cpp:309: bool hasLocalFrame =
> document().frame() ? true : false;
> On 2014/10/14 22:46:09, ojan-only-code-yellow-reviews wrote:
> > On 2014/10/14 22:43:07, adamk wrote:
> > > To answer Ojan's question, this is exactly the reason images "don't load"
in
> > > templates.
> > > 
> > > Ojan, are you asking for this check to move further up?
> > 
> > I don't know the code here well enough to know what the right thing to do
is,
> > but at first blush, it seems to me like we shouldn't parse sizes on images
in
> > templates. We'll reparse when we stamp out the new image, right?
> 
> Sounds like we should reparse when we switch documents (didMoveToNewDocument).

I agree.

[bzsolt, 2014-10-16 13:01:36]

https://codereview.chromium.org/653023002/diff/20001/Source/core/html/HTMLIma...
File Source/core/html/HTMLImageElement.cpp (right):

https://codereview.chromium.org/653023002/diff/20001/Source/core/html/HTMLIma...
Source/core/html/HTMLImageElement.cpp:362: return InsertionDone;
This modification caused to fail a test
(fast/dom/HTMLImageElement/image-srcset-deregistration-after-move.html) with an
assert (ASSERT(isInTreeScope()) in void
blink::Element::updateId(blink::TreeScope&, const WTF::AtomicString&, const
WTF::AtomicString&)) in Element.cpp, therefore I replaced the assert to an early
return. Is it good?

[Yoav Weiss, 2014-10-16 13:27:17]

Isn't simply bailing out of sourceSizeURL enough?

https://codereview.chromium.org/653023002/diff/20001/Source/core/html/HTMLIma...
File Source/core/html/HTMLImageElement.cpp (right):

https://codereview.chromium.org/653023002/diff/20001/Source/core/html/HTMLIma...
Source/core/html/HTMLImageElement.cpp:362: return InsertionDone;
On 2014/10/16 13:01:36, bzsolt wrote:
> This modification caused to fail a test
> (fast/dom/HTMLImageElement/image-srcset-deregistration-after-move.html) with
an
> assert (ASSERT(isInTreeScope()) in void
> blink::Element::updateId(blink::TreeScope&, const WTF::AtomicString&, const
> WTF::AtomicString&)) in Element.cpp, therefore I replaced the assert to an
early
> return. Is it good?

I don't know that this is correct. There are a lot of things happening in
insertedInto that are not related to source selection.

[Yoav Weiss, 2014-10-16 13:39:37]

On 2014/10/16 13:27:17, Yoav Weiss wrote:
> Isn't simply bailing out of sourceSizeURL enough?
> 
No, it isn't.
Maybe add an active document check right next to pictureEnabled() in
insertedInto?

[bzsolt, 2014-10-16 13:43:16]

On 2014/10/16 13:27:17, Yoav Weiss wrote:
> Isn't simply bailing out of sourceSizeURL enough?
> 
>
https://codereview.chromium.org/653023002/diff/20001/Source/core/html/HTMLIma...
> File Source/core/html/HTMLImageElement.cpp (right):
> 
>
https://codereview.chromium.org/653023002/diff/20001/Source/core/html/HTMLIma...
> Source/core/html/HTMLImageElement.cpp:362: return InsertionDone;
> On 2014/10/16 13:01:36, bzsolt wrote:
> > This modification caused to fail a test
> > (fast/dom/HTMLImageElement/image-srcset-deregistration-after-move.html) with
> an
> > assert (ASSERT(isInTreeScope()) in void
> > blink::Element::updateId(blink::TreeScope&, const WTF::AtomicString&, const
> > WTF::AtomicString&)) in Element.cpp, therefore I replaced the assert to an
> early
> > return. Is it good?
> 
> I don't know that this is correct. There are a lot of things happening in
> insertedInto that are not related to source selection.

In this testcase its enough, but in other case, I don't know.

[Yoav Weiss, 2014-10-20 14:20:49]

On 2014/10/16 13:43:16, bzsolt wrote:
> On 2014/10/16 13:27:17, Yoav Weiss wrote:
> > Isn't simply bailing out of sourceSizeURL enough?
> > 
> >
>
https://codereview.chromium.org/653023002/diff/20001/Source/core/html/HTMLIma...
> > File Source/core/html/HTMLImageElement.cpp (right):
> > 
> >
>
https://codereview.chromium.org/653023002/diff/20001/Source/core/html/HTMLIma...
> > Source/core/html/HTMLImageElement.cpp:362: return InsertionDone;
> > On 2014/10/16 13:01:36, bzsolt wrote:
> > > This modification caused to fail a test
> > > (fast/dom/HTMLImageElement/image-srcset-deregistration-after-move.html)
with
> > an
> > > assert (ASSERT(isInTreeScope()) in void
> > > blink::Element::updateId(blink::TreeScope&, const WTF::AtomicString&,
const
> > > WTF::AtomicString&)) in Element.cpp, therefore I replaced the assert to an
> > early
> > > return. Is it good?
> > 
> > I don't know that this is correct. There are a lot of things happening in
> > insertedInto that are not related to source selection.
> 
> In this testcase its enough, but in other case, I don't know.

All in all, looks good. Can you also add a test where a newly created Image is
added into a template element?

[bzsolt, 2014-10-20 15:41:18]

On 2014/10/20 14:20:49, Yoav Weiss wrote:
> On 2014/10/16 13:43:16, bzsolt wrote:
> > On 2014/10/16 13:27:17, Yoav Weiss wrote:
> > > Isn't simply bailing out of sourceSizeURL enough?
> > > 
> > >
> >
>
https://codereview.chromium.org/653023002/diff/20001/Source/core/html/HTMLIma...
> > > File Source/core/html/HTMLImageElement.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/653023002/diff/20001/Source/core/html/HTMLIma...
> > > Source/core/html/HTMLImageElement.cpp:362: return InsertionDone;
> > > On 2014/10/16 13:01:36, bzsolt wrote:
> > > > This modification caused to fail a test
> > > > (fast/dom/HTMLImageElement/image-srcset-deregistration-after-move.html)
> with
> > > an
> > > > assert (ASSERT(isInTreeScope()) in void
> > > > blink::Element::updateId(blink::TreeScope&, const WTF::AtomicString&,
> const
> > > > WTF::AtomicString&)) in Element.cpp, therefore I replaced the assert to
an
> > > early
> > > > return. Is it good?
> > > 
> > > I don't know that this is correct. There are a lot of things happening in
> > > insertedInto that are not related to source selection.
> > 
> > In this testcase its enough, but in other case, I don't know.
> 
> All in all, looks good. Can you also add a test where a newly created Image is
> added into a template element?

Yes of course, I have updated.

[Yoav Weiss, 2014-10-21 08:49:42]

On 2014/10/20 15:41:18, bzsolt wrote:
> On 2014/10/20 14:20:49, Yoav Weiss wrote:
> > On 2014/10/16 13:43:16, bzsolt wrote:
> > > On 2014/10/16 13:27:17, Yoav Weiss wrote:
> > > > Isn't simply bailing out of sourceSizeURL enough?
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/653023002/diff/20001/Source/core/html/HTMLIma...
> > > > File Source/core/html/HTMLImageElement.cpp (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/653023002/diff/20001/Source/core/html/HTMLIma...
> > > > Source/core/html/HTMLImageElement.cpp:362: return InsertionDone;
> > > > On 2014/10/16 13:01:36, bzsolt wrote:
> > > > > This modification caused to fail a test
> > > > >
(fast/dom/HTMLImageElement/image-srcset-deregistration-after-move.html)
> > with
> > > > an
> > > > > assert (ASSERT(isInTreeScope()) in void
> > > > > blink::Element::updateId(blink::TreeScope&, const WTF::AtomicString&,
> > const
> > > > > WTF::AtomicString&)) in Element.cpp, therefore I replaced the assert
to
> an
> > > > early
> > > > > return. Is it good?
> > > > 
> > > > I don't know that this is correct. There are a lot of things happening
in
> > > > insertedInto that are not related to source selection.
> > > 
> > > In this testcase its enough, but in other case, I don't know.
> > 
> > All in all, looks good. Can you also add a test where a newly created Image
is
> > added into a template element?
> 
> Yes of course, I have updated.

LGTM

Thanks for fixing this! :)

[Mike West, 2014-10-21 08:52:03]

mkwst@chromium.org changed reviewers:
+ mkwst@chromium.org

[Mike West, 2014-10-21 08:52:04]

rs lgtm because Yoav told me to.

[Mike West, 2014-10-21 08:52:08]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2014-10-21 08:52:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/653023002/100001

[commit-bot: I haz the power, 2014-10-21 11:00:53]

Committed patchset #6 (id:100001) as 184070