New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dedup_inline_images filter is broken #1369

Closed
jmarantz opened this Issue Aug 1, 2016 · 10 comments

Comments

Projects
None yet
3 participants
@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Aug 1, 2016

From a scan of the email I thought at first this might be #1002, but this actually looks like the filter is seriously broken now?

@morlovich

This comment has been minimized.

Copy link
Contributor

morlovich commented Aug 4, 2016

Seems easier to reproduce with local debug install than mps.com due to lack
of critical image stuff getting involved. Taking a look.

On Mon, Aug 1, 2016 at 6:43 PM, Jeff Kaufman notifications@github.com
wrote:

From a scan of the email I thought at first this might be #1002
#1002, but this
actually looks like the filter is seriously broken now?


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#1369 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ADl1RLV96ohc6-p7VEvoYY8X66Eb5WcNks5qbnZ5gaJpZM4JZqal
.

@morlovich

This comment has been minimized.

Copy link
Contributor

morlovich commented Aug 4, 2016

Well, this is obviously broken (irrelevant stuff removed) --- why are we getting all the duplicated (including the one that has the data) the same ID?:
<script id="pagespeed_script_1" data-pagespeed-no-defer>
pagespeed.dedupInlinedImages.inlineImg('pagespeed_img_98KheFixjv','pagespeed_img_98KheFixjv','pagespeed_script_1');
</script>

<script type="text/javascript" id="pagespeed_script_2" data-pagespeed-no-defer>
pagespeed.dedupInlinedImages.inlineImg('pagespeed_img_98KheFixjv','pagespeed_img_98KheFixjv','pagespeed_script_2');

@morlovich

This comment has been minimized.

Copy link
Contributor

morlovich commented Aug 4, 2016

This might be enough (but I haven't run any actual tests):
--- net/instaweb/rewriter/dedup_inlined_images_filter.cc 2015-08-28 14:25:48.000000000 -0400
+++ net/instaweb/rewriter/dedup_inlined_images_filter.cc 2016-08-04 11:29:01.000000000 -0400
@@ -117,7 +117,8 @@
GoogleString element_id;
const char* id = element->AttributeValue(HtmlName::kId);
if (id == NULL || id[0] == '\0') {

  •  element_id = StrCat("pagespeed_img_", hash);
    
  •  element_id = StrCat("pagespeed_img_", hash,
    
  •                      IntegerToString(++snippet_id_));
    
    driver()->AddAttribute(element, HtmlName::kId, element_id);
    } else {
    element_id = id;
@morlovich

This comment has been minimized.

Copy link
Contributor

morlovich commented Aug 4, 2016

Let's try this again:

--- net/instaweb/rewriter/dedup_inlined_images_filter.cc      2015-08-28 14:25:48.000000000 -0400
+++ net/instaweb/rewriter/dedup_inlined_images_filter.cc       2016-08-04 11:29:01.000000000 -0400
@@ -117,7 +117,8 @@
     GoogleString element_id;
     const char* id = element->AttributeValue(HtmlName::kId);
     if (id == NULL || id[0] == '\0') {
-      element_id = StrCat("pagespeed_img_", hash);
+      element_id = StrCat("pagespeed_img_", hash,
+                          IntegerToString(++snippet_id_));
       driver()->AddAttribute(element, HtmlName::kId, element_id);
     } else {
       element_id = id;

morlovich added a commit that referenced this issue Aug 4, 2016

Make dedup_image_filter work in cases where we generate IDs:
don't produce duplicate IDs for all the duplicates.


Should address
#1369
@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Aug 5, 2016

Any idea how this could have gotten broken? Did it just never work?

@jmarantz

This comment has been minimized.

Copy link
Contributor

jmarantz commented Aug 5, 2016

I haven't dug in deeply enough to be sure but I assume it worked at one
point. One possible explanation is that maybe browsers were sloppier then
about allowing multiple elements with the same ID.

On Fri, Aug 5, 2016 at 1:48 AM, Jeff Kaufman notifications@github.com
wrote:

Any idea how this could have gotten broken? Did it just never work?


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#1369 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AB2kPXjO0ZPkBi1ee8Lt_FmyNffXSeScks5qcs61gaJpZM4JZqal
.

@morlovich

This comment has been minimized.

Copy link
Contributor

morlovich commented Aug 5, 2016

That would be very weird, since it wass basically doing
document.getElementById(to_id).src = document.getElementById(from_id).src, where to_id was equal to from_id.

crowell added a commit that referenced this issue Aug 9, 2016

Make dedup_image_filter work in cases where we generate IDs:
don't produce duplicate IDs for all the duplicates.


Should address
#1369

@crowell crowell changed the title dedup_inline_images filter appears to be broken dedup_inline_images filter is broken Aug 12, 2016

@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Oct 3, 2016

This went out in 1.11.33.3

@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Oct 3, 2016

confirmed fixed

@jeffkaufman jeffkaufman closed this Oct 3, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment