Saving as "Web page, complete" on page with 2+ external resources causes hang in nsWebBrowserPersist::CalculateUniqueFilename

VERIFIED FIXED in mozilla1.9.2a1

Status

()

--
critical
VERIFIED FIXED
10 years ago
10 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

(4 keywords)

Trunk
mozilla1.9.2a1
dogfood, hang, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments)

(Assignee)

Description

10 years ago
Created attachment 358475 [details]
testcase

STR:
 1. Load testcase
 2. Ctrl+S (or File | Save Page As) -- ensure "Web Page, complete" is selected
 3. Click "Save"

ACTUAL RESULTS:
 Hang with 100% CPU usage.

NOTES: If I use an alternate testcase with only 1 image, I get no hang.  However, if I add an external stylesheet, I get the hang again. I haven't tried other combinations, but I'm guessing that we basically save the first external resource OK but have a problem on the second one.

Breaking in in GDB shows that we hang in the main "while(1)" loop of nsWebBrowserPersist::CalculateUniqueFilename -- we never leave that loop.
(Assignee)

Comment 1

10 years ago
Looks like this was caused by bug 466622 -- specifically, now that mFilenameList is a nsTArray (changed in that bug), its IndexOf method returns unsigned values, which means it returns +infinity on failure (not -1).

As a result, this line now fails:
 2033             // Test if the name is a duplicate
 2034             if (mFilenameList.IndexOf(tmpPath) < 0)
 2035             {

(I wonder if there were other similar regressions caused by that bug?)
Assignee: nobody → dholbert
Blocks: 466622
(Assignee)

Comment 2

10 years ago
Created attachment 358479 [details] [diff] [review]
fix: use nsTArray's "Contains" method

This should fix the problem by using the "Contains" method, which is what that IndexOf check was really trying to find out.

I'm also replacing another "mFilenameList.Length() > 0" check with "!mFilenameList.IsEmpty()", as a minor cleanup.

recompiling & testing...
(Assignee)

Comment 3

10 years ago
Comment on attachment 358479 [details] [diff] [review]
fix: use nsTArray's "Contains" method

This fixes the hang on my machine.
Attachment #358479 - Flags: superreview?(roc)
Attachment #358479 - Flags: review?(roc)
Attachment #358479 - Flags: superreview?(roc)
Attachment #358479 - Flags: superreview+
Attachment #358479 - Flags: review?(roc)
Attachment #358479 - Flags: review+
Can we write a test for that code?
(Assignee)

Comment 5

10 years ago
(In reply to comment #4)
> Can we write a test for that code?

I have no idea -- I don't know if we have any testing frameworks set up for our file-saving code.  Maybe someone from QA or places might know?
(Assignee)

Updated

10 years ago
Keywords: dogfood
(In reply to comment #5)
> (In reply to comment #4)
> > Can we write a test for that code?
> 
> I have no idea -- I don't know if we have any testing frameworks set up for our
> file-saving code.  Maybe someone from QA or places might know?

Can't that be a xpshell test? 

Btw. it happens too on Windows.
Severity: normal → critical
Status: NEW → ASSIGNED
OS: Linux → All
Hardware: x86 → All
Blocks: 441052
(Assignee)

Comment 7

10 years ago
(In reply to comment #6)
> Can't that be a xpshell test? 

Maybe -- I'm not particularly familiar with that suite of tests.  Henrik, do you think you could write one, or perhaps CC someone who could?

Also, FWIW -- I just fired off a tryserver build with this patch -- should be ready in an hour or two. I'm hoping that build can mitigate this bug's blockage of progress on bug 441052 and bug 474886.
(Assignee)

Comment 8

10 years ago
Comment on attachment 358479 [details] [diff] [review]
fix: use nsTArray's "Contains" method

I'm requesting approval to land this patch **on mozilla-central**, even though we're in a restricted-tree state, because:
 [1] this bug is a fairly serious dogfood issue which blocks useful work
 [2] the patch is really simple & safe, clearly restoring correct behavior

I'm requesting approval via the "approval1.9.1" flag, even though this patch isn't intended for 1.9.1, because there _is_ no "approvalMozilla-Central" flag.  (but let me know if there's a more correct way to request approval to land on mozilla-central during this restricted-tree time)

** Justifications for statements above:
[1] seriousness: This bug hinders us from reducing testcases and tracking down regression ranges. (see for example bug 474886 comment 7.)  Hence, it inhibits work on bugs in mozilla-central that _could_ affect 1.9.1 (i.e. tracking down regressions from patches that are baking in mozilla-central, awaiting landing on 1.9.1)
[2] safety / simplicity: see description of issue & fix in comment 1 & comment 2 -- this is a really simple regression, and this patch simply restores the prior correct behavior.
Attachment #358479 - Flags: approval1.9.1?
(In reply to comment #7)
> Maybe -- I'm not particularly familiar with that suite of tests.  Henrik, do
> you think you could write one, or perhaps CC someone who could?

Martijn, can you help us here? At least by giving a name who can write such a test?
Comment on attachment 358479 [details] [diff] [review]
fix: use nsTArray's "Contains" method

a=beltzner for mozilla-central landing, yeah. Not gonna set the 191 flag (though I understand why it was used) just so it doesn't mess with future queries that look for a191 but not fixed191 :)
Attachment #358479 - Flags: approval1.9.1?
(Assignee)

Updated

10 years ago
Flags: in-testsuite?
(Assignee)

Comment 11

10 years ago
Pushed to mozilla-central: http://hg.mozilla.org/mozilla-central/rev/ad84355b19bc
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Thanks Daniel! Verified with:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090128 Minefield/3.2a1pre ID:20090128020342

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2a1pre) Gecko/20090128 Minefield/3.2a1pre ID:20090128034731
Status: RESOLVED → VERIFIED
Target Milestone: --- → mozilla1.9.2a1
Created attachment 359550 [details] [diff] [review]
browserchrome test

This is the beginning of a browserchrome test, it hangs nicely in my debug build, which didn't contain the patch yet.
But probably the test should be improved, for instance to test whether the page was correctly saved and remove the saved page afterwards (otherwise you get such a mess).
You need to log in before you can comment on or make changes to this bug.