621253 - "Extension"/"Plugins" pane of "Add-ons Manager" is drawn wrongly if zoom level not equal 100% in site specific zoom and disabled extension/plugin existed.

Status: VERIFIED FIXED

(Core :: SVG, defect)

Comment 1

[Alice0775 White]

Attachment: extensions folder (maybe all extensions)

Sample extentions folder.
Note:
Containing extensions are disabled by default because of maxVersion less tan 4.0b9pre.

Comment 2

[Alice0775 White]

Build Identifier:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b9pre) Gecko/20101222 Firefox/4.0b9pre ID:20101222030351

"Extension" pane of "Add-ons Manager" is drawn wrongly  if zoom level does not equal 100%  in site specific zoom.


Reproducible: Always

Steps to Reproduce:
1. Start Minefield with new profile
2. Open Add-ons Manager(Ctrl+Shift+A)
3. Zoom out (Ctrl++ Ctrl++ Ctrl++)
4. Install Several Addons
4'.Alternative, Exit Browser and Copy extensions folder(in the attached zip) to profile folder. And Restart browser
5. Open Add-ons Manager(Ctrl+Shift+A)

Actual Results:
 "Extension" pane of "Add-ons Manager" is drawn wrongly

Expected Results:
 "Extension" pane of "Add-ons Manager" is drawn in specified zoom level

Regression window:
Works:
http://hg.mozilla.org/mozilla-central/rev/a2a3a6e8b0e0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b9pre) Gecko/20101221 Firefox/4.0b9pre ID:20101221145009
Fails:
http://hg.mozilla.org/mozilla-central/rev/52b229f6a051
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b9pre) Gecko/20101221 Firefox/4.0b9pre ID:20101221150740
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a2a3a6e8b0e0&tochange=52b229f6a051

Comment 3

[Alice0775 White]

Attachment: screenshot

Comment 4

[Alice0775 White]

Attachment: sample extensons folder

reduced extensions

Comment 5

[Alice0775 White]

It happens if disabled extention/plugin existed in the Manager.

Comment 6

[Alice0775 White]

It happens on Linux too.
Mozilla/5.0 (X11; Linux i686; rv:2.0b9pre) Gecko/20101223 Firefox/4.0b9pre ID:20101223030401


In local build.
Build from 6b98f6cf4589 : fails
Build from 95bbe1076dcb : fails
Build from a2a3a6e8b0e0 : works:

Regressed by 95bbe1076dcb	Daniel Holbert — Bug 611241: Allow SVG filters from external documents to be used in about: pages. r=bzbarsky a=blocking-betaN+

Comment 7

[Alice0775 White]

This happens on attachment 490778 [details] of Bug 611241 too.

On latest nightly
  Open about:bug611241 and zoom In
  Reload --- zoom reset, NG
  Switch tabs --- zoom level as expected

On build prior to landing of Bug 611241 (Of course the first image is not displayed)
  Open about:bug611241 and zoom In
  Reload and  Switch tabs--- no problem

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This sounds like it may be related to bug 541270 (although it looks a bit different).

Comment 9

[Ken]

Attachment: Add-on manager zoom problem

At least on XP; The full page displays at the correct zoom level first, then the inset (circled area) is displayed after that.
Then if you get the tab to redisplay (ex: press Reload button; switch off tab, then go back...), what is left is what Alice screenshot shows.

Also if you look closely at the image, the larger correctly zoomed screen does not have the images displayed that Bug 611241 fixed.
 So it looks like code that displays the about:addons (without images) executes first, the code from Bug 611241 comes and overlays it with a non-zoomed page that only uses only what 100% would use off the zoomed level (ex: if your zoom level for the Add-ons page is set to 200% that displays first then a screen 1/2 its size displays over it).
 In the attached example I have the Zoom level set to 140%.

Comment 10

[Daniel Holbert [:dholbert]]

I can reproduce this using the Simple Testcase extension, attached to bug 611241. (at both the about: and chrome: URLs in bug 611241 comment 4)

Comment 11

[Daniel Holbert [:dholbert]]

(In reply to comment #8)
> This sounds like it may be related to bug 541270 (although it looks a bit
> different).

Yeah, I think they're roughly the same bug.

Comment 12

[Daniel Holbert [:dholbert]]

Or at least that bug is part of the brokenness that now affects about:addons.  The brokenness on tab-switch & reload (not described in that bug) may be xul-specific.

Comment 13

[Daniel Holbert [:dholbert]]

The reload & tab-switch issues in this bug (the most broken part) only happens for content with a filter defined in an external resource.  So it happens for about:addons (which outsources its filter), but not for the testcases on bug 541270 (which use locally-embedded filters).

I've filed bug 621639 on the reload & tab-switch issues, with a reduced XHTML testcase.

IIUC, that's the main thing that's busted here.  (The part covered by bug 541270 actually just means that the icons for disabled addons will be cropped when you zoom out, and that's not as annoying of a problem.)

So since bug 621639 is specific to external resources, we could *almost* hack around it for about:addons by using a locally-defined filter.  However, that workaround can't actually work for about:addons, because local references are busted for about: pages, as bz noted in bug 611241 comment 5.

Comment 15

[Daniel Holbert [:dholbert]]

(In reply to comment #13)
> I've filed bug 621639 on the reload & tab-switch issues, with a reduced XHTML
> testcase.
> IIUC, that's the main thing that's busted here.

bug 621639 is indeed the cause of the issue from comment 2 here -- so I just duped bug 621639 back to this bug.

I have a WIP patch -- coming up soon.

Comment 16

[Daniel Holbert [:dholbert]]

Attachment: fix

This patch sets the External Resource's zoom levels to match those of its display document *when it's created*.

This change is necessary because the initial calls to DocumentViewerImpl::SetFullZoom() & SetTextZoom() for the display document happen *before* our external resources are loaded, and so their well-meaning "EnumerateExternalResources()" chunks don't actually accomplish anything (as far as this bug is concerned) at page-load time. (See bug 621639 comment 6 & the comments leading up to it for more.)

Comment 17

[Daniel Holbert [:dholbert]]

The patch includes a reftest based on the testcase in simplified/duplicate bug 621253.  It just compares full-page-zoom on content with an external filter vs. content with a locally-defined filter.  I confirmed with a local reftest-run that it fails pre-patch & passes post-patch.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 502706 [details] [diff] [review]
fix

r=me.  Sorry for the lag...

Comment 19

[Daniel Holbert [:dholbert]]

No problem, thanks for the review!
Landed: http://hg.mozilla.org/mozilla-central/rev/0609193325c9

Comment 20

[Daniel Holbert [:dholbert]]

backed out due to crashes during unittest runs.  Looks like one or the other document is null.  (I didn't think either could be, but I guess so (?)):
http://hg.mozilla.org/mozilla-central/rev/9ab122e89d62

Probably just needs a null-check somewhere, but I want to understand why the document's null before adding the null-check...

Comment 21

[Daniel Holbert [:dholbert]]

Aha! So earlier I'd had the "TransferZoomLevels()" call up inside of the "if (aViewer)" clause, where we set |doc| and then proceed assuming it's non-null.

However, once we've left that clause, we no longer no for sure that |doc| (the external resource doc) is non-null.  It gets explicitly nulled out in a few error-checking clauses, for example.

So we need to null-check |doc|. before calling TransferZoomLevels.  I think that should do it, but I'll run the amended patch through TryServer to be sure...

Comment 22

[Daniel Holbert [:dholbert]]

(In reply to comment #21)
> However, once we've left that clause, we no longer no for sure that |doc| [...]

(er "know for sure")

Comment 23

[Daniel Holbert [:dholbert]]

Incidentally, it looks like at least one of the unit test files[1] that triggered this failure had a null external-resource-doc because it's using a remote filter that gets blocked due to security restrictions.

I'm guessing that's not what the test-writers were expecting -- this is likely a mistake in the tests... (though I'm glad of the mistake in this case, because it caught this issue :))

[1] layout/reftests/bugs/621918-2-ref.svg

Comment 24

[Daniel Holbert [:dholbert]]

Ah, the mochitest-1 crash[1] is from intentionally using a nonexistent mask:
> style="mask: url(no-such-document-I-tell-you#x"
so comment 23 doesn't apply to it. :)
[1] http://mxr.mozilla.org/mozilla-central/source/content/base/test/test_bug459424.html?force=1

Comment 25

[Daniel Holbert [:dholbert]]

I filed bug 625984 to investigate comment 23.

Comment 26

[Boris Zbarsky [:bzbarsky]]

> Ah, the mochitest-1 crash[1] is from intentionally using a nonexistent mask:

That's a test we added for the "Null-deref crash when zooming on page with invalid external resource documents" bug.  ;)

Comment 27

[Daniel Holbert [:dholbert]]

Re-landed with a null-check before calling TransferZoomLevels, and with an ABORT_IF_FALSE inside of TransferZoomLevels. (sanity-checked on TryServer, too.)
   http://hg.mozilla.org/mozilla-central/rev/031f6302fcae

Comment 28

[Alex Lakatos[:AlexLakatos]]

Attachment: screenshot-2011-01-20

The problem Alice reported seems to be fixed on Build identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.0b10pre) Gecko/20110120 Firefox/4.0b10pre, but this new one appears:

On maximum zoom, the buttons are partially hidden and the horizontal scroll does not fully scroll down.

Comment 29

[Alice0775 White]

(In reply to comment #28)
> Created attachment 505791 [details]
> screenshot-2011-01-20
> 
> The problem Alice reported seems to be fixed on Build identifier: Mozilla/5.0
> (Windows NT 5.1; rv:2.0b10pre) Gecko/20110120 Firefox/4.0b10pre, but this new
> one appears:
> 
> On maximum zoom, the buttons are partially hidden and the horizontal scroll
> does not fully scroll down.

This is not related to this bug.

Comment 30

[Daniel Holbert [:dholbert]]

I filed Bug 627768 on comment 28.