582795 - Changing encoding in view source doesn't update page

Status: RESOLVED FIXED

(Toolkit :: View Source, defect)

Description

[Emil Ivanov]

from bug 582788
WIth Firefox 4 b2 when changing encoding in View source, nothing happens until i refresh manually.

Comment 1

[Dietrich Ayala (:dietrich)]

Can someone please contribute a confirmation and a regression range?

Comment 2

[Alice0775 White]

Regression window:
Works:
http://hg.mozilla.org/mozilla-central/rev/8c1aa8a2485c
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a1pre) Gecko/20100201 Minefield/3.7a1pre ID:20100201094116
Fails:
http://hg.mozilla.org/mozilla-central/rev/5c6c40887584
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a1pre) Gecko/20100201 Minefield/3.7a1pre ID:20100201174142
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8c1aa8a2485c&tochange=5c6c40887584

I revert to afff5c13d296 in local build, then the problem is gone.
So, Changeset a2f468118868 of Bug 500328 causes the problem.

Comment 3

[Justin Lebar (not reading bugmail)]

I'll have a look at what's going on.

Comment 4

[Justin Lebar (not reading bugmail)]

Alice or Emil, could you give please link me to a page to test this on?

Comment 5

[Emil Ivanov]

from bug 582788
http://cda.ipmailing.it/IPMailing/forms/optInForm1.asp

Comment 6

[Alice0775 White]

(In reply to comment #4)
> Alice or Emil, could you give please link me to a page to test this on?
http://mozilla.jp/firefox/  -- this is UTF-8 page. and I changed to Western (ISO-8859-1) in viewsource window and confirmed the issue.

Comment 7

[Emil Ivanov]

Yes, actually you can do it from any page including in this bug, just change it to something different than the auto detected.

Comment 8

[Emil Ivanov]

BTW the list with recent used encodings is not updated after using another encoding, maybe the problem is in this bug. In Firefox 3.6.8 i have all kinds of UTF when testing, but in

Comment 9

[Emil Ivanov]

BTW the list with recent used encodings is not updated after using another encoding, maybe the problem is in this bug. In Firefox 3.6.8 i have all kinds of UTF when testing, but in latest nightly there are only the original two Western and UTF-8.

ps sorry for my previous post i clicked accidentally on 'save changes' button when trying to close the view source...

Comment 10

[Justin Lebar (not reading bugmail)]

I can confirm that this regressed with changeset a2f468118868.  I have no idea why, but I'll look into it.

Comment 11

[Justin Lebar (not reading bugmail)]

Aha.  The problem is that pushState introduces the idea of navigating between two SHEntries with the same document identifier.  If the document identifier is the same, we short-circuit loads between them (just as we short-circuit loads between foo.html#bar and foo.html#baz) [1].

I wonder what the right fix is here.  We could increment the document identifier when we change the encoding, although we'd probably only want to do that for view-source URIs, as doing otherwise would break pushstate.

[1]: http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#7948

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

So what caused the reload before pushState? And what kind of reload?

Comment 13

[Dave Townsend [:mossop]]

Blocks final, please make sure it is tested though

Comment 14

[Justin Lebar (not reading bugmail)]

(In reply to comment #12)
> what kind of reload?

In nsDocShell::InternalLoad, aLoadType is LOAD_HISTORY.

> So what caused the reload before pushState?

Before the pushstate patch, InternalLoad did a regular, non-short-circuiting load instead of falling into the anchor load case.

Comment 15

[Justin Lebar (not reading bugmail)]

The load type comes from nsDocShell::LoadPage(), which is called from JS.  I'm not sure where in JS; I thought I just had to |call DumpJSStack()|, but that doesn't output anything.

Comment 16

[Justin Lebar (not reading bugmail)]

Oh.  gdb tui was messing me up.  The JS stack is

1 BrowserSetForcedCharacterSet(aCharset = "UTF-8") ["chrome://global/content/viewSource.js":675]                                                              
    this = [object ChromeWindow @ 0x230ab70 (native @ 0x2360398)]
2 SetForcedCharset(charset = "UTF-8") ["chrome://global/content/charsetOverlay.js":115]
    this = [object ChromeWindow @ 0x230ab70 (native @ 0x2360398)]

Comment 17

[Justin Lebar (not reading bugmail)]

Attachment: Partial fix

The view-source charset updates when you change it with this patch applied.  No idea if this breaks anything else...

The selected charset still doesn't show up in the list of recently-used charsets.  The obvious thing to try would be to make the same change BrowserSetForcedDetector(), but unfortunately that doesn't seem to work.  :)

Comment 18

[Justin Lebar (not reading bugmail)]

Comment on attachment 463423 [details] [diff] [review]
Partial fix

Actually, that patch fixes nothing.  I was testing on the wrong head. :(

In other news, changing the view-source encoding doesn't add that encoding to the recently-used list in changeset afff5c (the one right before the regression).  So maybe we don't have to worry about that here.

Comment 19

[Emil Ivanov]

Isnt it possible to reuse the code from the main menu bar?
Its working without any problems there.

Comment 20

[Justin Lebar (not reading bugmail)]

Attachment: Patch v1

This seems to fix the problem, although who knows what it breaks.  There are some comments nearby that seem anachronistic -- the first one apparently refers to the wrong bug.

How would one go about writing a test for this?  And who can we cc that understands this code so they can tell me why my change is a bad idea?  :)

Comment 21

[Justin Lebar (not reading bugmail)]

I don't think this should block, but if it does, it needs a testcase.  Olli or bz, do you have any idea how to test this?

Comment 22

[Boris Zbarsky [:bzbarsky]]

You can just loadPage in an arbitrary docshell, etc, no?

Comment 23

[Justin Lebar (not reading bugmail)]

(In reply to comment #22)
> You can just loadPage in an arbitrary docshell, etc, no?

Could you elaborate on this a bit?

Comment 24

[Boris Zbarsky [:bzbarsky]]

Load something in a frame.  Create another frame.  Get the latter's docshell.  loadPage the desscriptor from the first frame in the second frame's docshell.  Then change the encoding on the second docshell and see whether the DOM is correct?

Comment 25

[Justin Lebar (not reading bugmail)]

Sorry, Boris; I still don't quite grock.

From a mochitest (?), create two iframes.  Load a page into one iframe.  Into the second iframe, load the descriptor from the first page's docshell.  Then change the second frame's encoding and check the DOM.

Is the descriptor you speak of the viewsource:// URI corresponding to the URI I loaded into the first frame?

Comment 26

[Boris Zbarsky [:bzbarsky]]

> Is the descriptor you speak of the viewsource:// URI

No.  It's "pageCookie" gotten in BrowserViewSourceOfDocument here: http://hg.mozilla.org/mozilla-central/file/11e4faad5545/browser/base/content/browser.js#l2259

And it's used like so:
http://hg.mozilla.org/mozilla-central/file/11e4faad5545/toolkit/components/viewsource/content/viewSource.js#l188

Comment 27

[Justin Lebar (not reading bugmail)]

Okay, I got that to work.

But now how do I call viewSource.js's version of BrowserSetForcedCharacterSet()?

Comment 28

[Boris Zbarsky [:bzbarsky]]

You don't.  You copy and paste it into your code, I would think.

Comment 29

[Justin Lebar (not reading bugmail)]

Hm.  Is it even worth writing a testcase, then, considering that the only changes I made were to BrowserSetForcedCharacterSet and BrowserSetForcedDetector?

Comment 30

[Boris Zbarsky [:bzbarsky]]

Oh, I see.  I didn't realize you were doing all this on the chrome side only...

You could try to write a browser test that opens the viewsource window and calls the viewsource functions, I guess....  But maybe it's not worth it.

Comment 31

[Justin Lebar (not reading bugmail)]

I'm happy not testing it if it's going to be a pain and/or result in a fragile test.

Comment 32

[Justin Lebar (not reading bugmail)]

Comment on attachment 463661 [details] [diff] [review]
Patch v1

Requesting bz's review here because I have no idea who owns this code.

Comment 33

[Boris Zbarsky [:bzbarsky]]

So I assume you tested that this doesn't regress bug 229503 or bug 136322?  In particular, you're just backing out the fix for bug 136322, right?

Did you test on source of no-cache POST results?

Comment 34

[Justin Lebar (not reading bugmail)]

You know, when I originally wrote this, I looked up those bugs, but I must have mistyped both bug numbers, because neither pointed to anything related to view source.

I'll go do the testing I should have done.

Comment 35

[Justin Lebar (not reading bugmail)]

bug 136322 isn't regressed (that's obvious from testing for this bug).  bug 229503 isn't strictly regressed, but I'd like to test on a site where changing the auto-detect setting has an effect.

Will test no-cache POST results in a moment.

Comment 36

[Justin Lebar (not reading bugmail)]

With a no-cache POST result page, it pops up and asks if I want to re-post.  That's probably not what we want, is it?

Comment 37

[Boris Zbarsky [:bzbarsky]]

No, we don't.  Does it not do that without this patch?

Comment 38

[Justin Lebar (not reading bugmail)]

(In reply to comment #36)> With a no-cache POST result page, it pops up and asks if I want to re-post. > That's probably not what we want, is it?Turns out that you get the same prompt if you change the encoding of a no-cache POST result outside of view-source.  bz and I conferred on IRC and think that we can add this behavior to view-source and then fix it at once both inside and outside view-source.The only thing that should be left to do for this patch is test that it doesn't render auto-detect useless.

Comment 39

[Justin Lebar (not reading bugmail)]

(Sorry about the lack of linebreaks in comment 38.  Bug 596752.)> The only thing that should be left to do for this patch is> test that it doesn't render auto-detect useless.Simon, do you know of any pages where auto-detect charset should have an effect, or how to make one?  My attempts thus far have not been fruitful.

Comment 40

[Simon Montagu :smontagu]

You could try any of the test files for autodetection at extensions/universalchardet/tests/bug*.html (Using a local copy rather than mxr, because mxr sends a Content-Type header with charset, so autodetection won't kick in)

Comment 41

[Justin Lebar (not reading bugmail)]

Interesting.  Changing the auto-detect setting in view-source has no effect on trunk.

Comment 42

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Should this really block?  It's seen no activity in well over a month and (at least IMHO) isn't that important.

Comment 43

[Dietrich Ayala (:dietrich)]

No. This is a developer-oriented feature with a very simple workaround. It should not block the release. Would take a low-risk, reviewed patch with tests.

Comment 44

[Boris Zbarsky [:bzbarsky]]

> No. This is a developer-oriented feature with a very simple workaround.

What workaround?  There is no workaround that actually works.  Reloading is not a workaround, since it totally fails on POST requests.

Resetting the blocking flag, since comment 43 was based on incorrect premises...

Note that the underlying issue here is the same as in bug 585298, btw.  So it's really two blockers for the price of one.

Comment 45

[Justin Lebar (not reading bugmail)]

Attachment: Patch v2

bz, what do you think of this patch?  It also fixes bug 585298.

I'm not sure it's possible to properly test this with an automated test.  We could certainly write a manual test for this and for bug 585298, though.

Comment 46

[Boris Zbarsky [:bzbarsky]]

Why's the unique identifier in an else clause, not unconditional?

That comment below the if/else block doesn't make sense at that location...

Comment 47

[Justin Lebar (not reading bugmail)]

Attachment: Patch v3

Comment 48

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 492751 [details] [diff] [review]
Patch v3

Let's not condition this on the scheme either, ok?

Comment 49

[Justin Lebar (not reading bugmail)]

Attachment: Patch v4

sgtm.

Comment 50

[Justin Lebar (not reading bugmail)]

Attachment: Patch v4.1

Forgot to qref.

Comment 51

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 492816 [details] [diff] [review]
Patch v4.1

"independent of", and sold!

Comment 52

[Justin Lebar (not reading bugmail)]

http://hg.mozilla.org/mozilla-central/rev/35f5ea086390