Changing encoding in view source doesn't update page

RESOLVED FIXED

Status

()

RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: stream, Assigned: justin.lebar+bug)

Tracking

({qawanted, regression})

Trunk
x86
Windows XP
qawanted, regression
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(blocking2.0 final+)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

8 years ago
from bug 582788
WIth Firefox 4 b2 when changing encoding in View source, nothing happens until i refresh manually.
Can someone please contribute a confirmation and a regression range?
Keywords: qawanted, regressionwindow-wanted

Comment 2

8 years ago
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.
Blocks: 500328
(Assignee)

Comment 3

8 years ago
I'll have a look at what's going on.
(Assignee)

Comment 4

8 years ago
Alice or Emil, could you give please link me to a page to test this on?
(Assignee)

Updated

8 years ago
Keywords: regressionwindow-wanted

Comment 6

8 years ago
(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.
(Reporter)

Comment 7

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

Comment 8

8 years ago
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
(Reporter)

Comment 9

8 years ago
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...
(Assignee)

Comment 10

8 years ago
I can confirm that this regressed with changeset a2f468118868.  I have no idea why, but I'll look into it.
(Assignee)

Updated

8 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 11

8 years ago
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
So what caused the reload before pushState? And what kind of reload?
Blocks final, please make sure it is tested though
blocking2.0: ? → final+
Flags: in-testsuite?
(Assignee)

Comment 14

8 years ago
(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.
(Assignee)

Comment 15

8 years ago
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.
(Assignee)

Comment 16

8 years ago
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)]
(Assignee)

Comment 17

8 years ago
Created attachment 463423 [details] [diff] [review]
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.  :)
(Assignee)

Comment 18

8 years ago
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.
Attachment #463423 - Attachment is obsolete: true
(Reporter)

Comment 19

8 years ago
Isnt it possible to reuse the code from the main menu bar?
Its working without any problems there.
(Assignee)

Comment 20

8 years ago
Created attachment 463661 [details] [diff] [review]
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?  :)
(Assignee)

Comment 21

8 years ago
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?
You can just loadPage in an arbitrary docshell, etc, no?
(Assignee)

Comment 23

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

Could you elaborate on this a bit?
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?
(Assignee)

Comment 25

8 years ago
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?
(Assignee)

Comment 27

8 years ago
Okay, I got that to work.

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

Updated

8 years ago
Summary: Changing encoding is not reloading source automatically → Changing encoding in view source doesn't refresh page
You don't.  You copy and paste it into your code, I would think.
(Assignee)

Comment 29

8 years ago
Hm.  Is it even worth writing a testcase, then, considering that the only changes I made were to BrowserSetForcedCharacterSet and BrowserSetForcedDetector?
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.
(Assignee)

Comment 31

8 years ago
I'm happy not testing it if it's going to be a pain and/or result in a fragile test.
(Assignee)

Comment 32

8 years ago
Comment on attachment 463661 [details] [diff] [review]
Patch v1

Requesting bz's review here because I have no idea who owns this code.
Attachment #463661 - Flags: review?(bzbarsky)
(Assignee)

Updated

8 years ago
Summary: Changing encoding in view source doesn't refresh page → Changing encoding in view source doesn't update page
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?
(Assignee)

Comment 34

8 years ago
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.
(Assignee)

Updated

8 years ago
Attachment #463661 - Flags: review?(bzbarsky)
(Assignee)

Comment 35

8 years ago
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.
(Assignee)

Comment 36

8 years ago
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?
No, we don't.  Does it not do that without this patch?
(Assignee)

Comment 38

8 years ago
(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.
(Assignee)

Comment 39

8 years ago
(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.
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)
(Assignee)

Comment 41

8 years ago
Interesting.  Changing the auto-detect setting in view-source has no effect on trunk.
Should this really block?  It's seen no activity in well over a month and (at least IMHO) isn't that important.
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.
blocking2.0: final+ → -
> 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.
blocking2.0: - → final+
(Assignee)

Comment 45

8 years ago
Created attachment 492452 [details] [diff] [review]
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.
Attachment #463661 - Attachment is obsolete: true
Attachment #492452 - Flags: review?(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...
(Assignee)

Comment 47

8 years ago
Created attachment 492751 [details] [diff] [review]
Patch v3
Attachment #492452 - Attachment is obsolete: true
Attachment #492751 - Flags: review?(bzbarsky)
Attachment #492452 - Flags: review?(bzbarsky)
Comment on attachment 492751 [details] [diff] [review]
Patch v3

Let's not condition this on the scheme either, ok?
Attachment #492751 - Flags: review?(bzbarsky) → review-
(Assignee)

Comment 49

8 years ago
Created attachment 492805 [details] [diff] [review]
Patch v4

sgtm.
Attachment #492751 - Attachment is obsolete: true
Attachment #492805 - Flags: review?(bzbarsky)
(Assignee)

Comment 50

8 years ago
Created attachment 492816 [details] [diff] [review]
Patch v4.1

Forgot to qref.
Attachment #492805 - Attachment is obsolete: true
Attachment #492816 - Flags: review?(bzbarsky)
Attachment #492805 - Flags: review?(bzbarsky)
Comment on attachment 492816 [details] [diff] [review]
Patch v4.1

"independent of", and sold!
Attachment #492816 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 52

8 years ago
http://hg.mozilla.org/mozilla-central/rev/35f5ea086390
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Updated

7 years ago
Assignee: nobody → justin.lebar+bug
You need to log in before you can comment on or make changes to this bug.