Closed Bug 490035 Opened 12 years ago Closed 11 years ago

"Script is busy" ("Unresponsive Script") warning from places flush script (nsPlacesDBFlush.js) on initial migration / import

Categories

(Firefox :: Bookmarks & History, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 4.0b9
Tracking Status
blocking2.0 --- -

People

(Reporter: jimm, Unassigned)

References

Details

(Keywords: helpwanted, Whiteboard: [TSnappiness][status is comment 21][see Bug 492410][fixed-in-places])

Attachments

(2 files, 1 obsolete file)

Attached image script timeout
While testing bug 313879, I occasionally received this error (see screen shot) during initial migration.  

This is also present in beta 4 as I just received it again while testing for bug 490009.
Flags: blocking-firefox3.5?
I should also add, clicking continue finishes up the import and everything continues on normally.
Whiteboard: [TSnappiness]
So, that's onItemChanged...  Hrm - seems like stuff isn't in a batch update?  Mak - any comments?  I don't understand how _flushWithQueries could be taking much time at all though...

...that is, unless are not in a batch update.
looks crazy to me, have to look at the code. that point does not look like can cause a script busy, looks like we call a bunch of onItemChanged and that causes an high possibility the code ends up locking in that method.
Tentatively blocking, though this may be more due to a change in how we do runaway script detection.

Jim: can you run a comparison with Firefox 3.0.10 to see if we're much slower than we used to be on migration?
Flags: blocking-firefox3.5? → blocking-firefox3.5+
(In reply to comment #4)
> Tentatively blocking, though this may be more due to a change in how we do
> runaway script detection.
> 
> Jim: can you run a comparison with Firefox 3.0.10 to see if we're much slower
> than we used to be on migration?

3.5 did the import in about half the time based on simple test using a sidebar stop watch.

I think one of the problems here is that the migration wizard doesn't have any user feedback while the import takes place. The windows message pump appears frozen - Vista reports the wizard as ("Not Responding)" in the titlebar after a few seconds. Maybe we need to add a progress to the import dialog and pump messages to it to free up events? That might give the script engine some time to breath as well.
Flags: in-litmus?
also bug 392193 could help here, there's a bitrotted patch there, we could try to strip part of it to speed up import, clearly not the interfaces changes.
Looks at these related bugs (dupes?):

Bug 489274
Bug 477070

Why does this one block but these older ones did not?  FWIW, I've personally seen the unresponsive script when importing bookmarks on 3.0.  Given a large enough import, hasn't this always happened?
this is happening more often in 3.5/3.6 due to changes in how the "slow script" warning works, that is probably the only difference with 3.0.x (so actually could probably happen with a not-so-large import file).
Looking at the code in nsIEPRofileMigrator, we use batch mode for history, but not for bookmarks.
Initially i was thinking to an import from html, but this is different, so nevermind comment 6.
We should at least import bookmarks in batch.
Attached patch unbitrot patch from bug 392193 (obsolete) — Splinter Review
this is the first patch Dietrich posted in bug 392193 (Oct 2007!), that bug then changed to cover html import and the patch was still laying around. But the approach is still valid, i modified it to make it compile. He said it was crashing, but from the first test i did it didn't, so could be some crasher has gone.
Jim could you try with this patch and see if anything changes?
(In reply to comment #10)
> Created an attachment (id=376130) [details]
> unbitrot patch from bug 392193
> 
> this is the first patch Dietrich posted in bug 392193 (Oct 2007!), that bug
> then changed to cover html import and the patch was still laying around. But
> the approach is still valid, i modified it to make it compile. He said it was
> crashing, but from the first test i did it didn't, so could be some crasher has
> gone.
> Jim could you try with this patch and see if anything changes?

Hmm, strange, this time it's in the event handler for the observer:

Script: chrome://browser/content/migration/migration.js:454

http://mxr.mozilla.org/mozilla-central/source/browser/components/migration/content/migration.js#430
we want that patch even if the slow script does not go away, and applied to all migrators. Maybe with an additional progressbar so we can get rid of that.
Probably there's always the same amount ot ticks before the warning fires, since we have removed the calls to dbflush using batch the warning has moved later in the code.
Does it take less time to complete the import?
(In reply to comment #12)

> Does it take less time to complete the import?

Yes, it was very quick!
Assignee: nobody → mak77
Jim, do you have an IE profile i can use to reproduce the hang?
Attached file ie favorites
Simplest way to reproduce it is to take a large folder of favorites with long titles (attached are some good msdn links) and duplicate that about 10 times in the favorites folder. I've used this to reproduce in an xp image.
Duplicate of this bug: 492049
Duplicate of this bug: 477070
Duplicate of this bug: 489274
this problem is quite common, we are hitting the disk with heavy load and the "Unresponsive script" dialog is a pain.
I've tried to enqueue import functions calls to the main thread to give some breath to the UI, potentially works between import steps, but as soon as we are importing a large history or large number of bookmarks, we are stuck inside a single import method, and i don't see clear ways to stop us doing work and dispatch to the main thread so that UI updates. unless we try to chunk the method so that it imports a maximum number of items per chunk.

This is a pain even when you remove a big number of bookmarks, i'd prefer that "Unresponsive script" warning being deactivable for code that is known taking time or for chrome code, or whatever (The dialog also says "a script ON THIS PAGE may be busy", but clearly it's not a script from the page we are on)
Attachment #376130 - Attachment is obsolete: true
i will provide batch migration patch in bug 392193, that will make import faster.

On the other side even if import will be faster the "unresponsive script" warning will still show with a lot of bookmarks, so we need to find a way to fix that here. That could be hard if we are not going to allow idl changes for migration interface.

the low-bar workaround is to temporarly increase the timeout pref value.
I'm doing some experiment with events enqueueing on the main thread and a dedicated observer interface, but any suggestion is welcome.

Simply adding a progressbar won't help a lot since the ui message pump is completely frozen as Jim pointed out.
Depends on: 392193
Whiteboard: [TSnappiness] → [TSnappiness][status is comment 20]
Blocks: 481327
With patch in bug 392193 importing bookmarks should take a few seconds on an optimized build (i tried with more than 10000 bookmarks), instead of a few minutes, so this could be no more an issue for a normal sized import (IIRC default chrome timeout is 20 seconds).
So i suggest taking that patch. Migration tests will require some more attention from QA (Migration components has sadly no automatic tests).

I tried other experiments to try breaking at better points the import without touching the interface, but still no luck so far, the maximum i could do is splitting between different import tasks (cookies, history, bookmarks, ...), but we will still lock during the execution of one of those.

I've no more ideas, so if anyone has any, feel free to steal the bug or suggest them.
Keywords: helpwanted
Whiteboard: [TSnappiness][status is comment 20] → [TSnappiness][status is comment 21]
jmathies, can you try your migration scenario again now that bug 392193 has landed?
(In reply to comment #22)
> jmathies, can you try your migration scenario again now that bug 392193 has
> landed?

Hmm, unfortunately I still receive the warning - 

Script: chrome://browser/content/migration/migration.js:457

It always happens right before the import finishes up.

This is on a debug 191 build that was checked out about an hour ago.
So, debug builds are going to be a bit slower than opt builds, so that actually matters.
we can probably re-evaluate blocking status due to the fact:
- we are quite faster than 3.0 and now quite faster then before bug 392193
- bug 492410

and this should really be fixed for .next.
Whiteboard: [TSnappiness][status is comment 21] → [TSnappiness][status is comment 21][see Bug 492410]
(In reply to comment #25)
> we can probably re-evaluate blocking status due to the fact:
> - we are quite faster than 3.0 and now quite faster then before bug 392193
> - bug 492410
I agree - let's put this back in the nomination queue.
Flags: blocking-firefox3.5+ → blocking-firefox3.5?
Agreed; removed & renomed for 192
Flags: blocking-firefox3.6?
Flags: blocking-firefox3.5?
Flags: blocking-firefox3.5-
Depends on: 492410
Is this still a problem?
potentially it is. I mean, we don't notify anymore for slow script, and migration is damn faster. in some case the warning can still appear because all migration work is done in a sequential way, all in the same method, all on the main thread.
(In reply to comment #29)
> I mean, we don't notify anymore for slow script, and

...

> in some case the warning can still appear because all

Which one is it? Pick a side, we're at war. :)
on really large migrations, for example heavy IE users having large profiles and tons of bookmarks.
Not blocking 3.6, though we'd take a fix. Is there a way to get this to work off the main thread such that it doesn't block? Or to at least show a progress indicator when it takes more than 2s?
blocking2.0: --- → ?
Flags: wanted-firefox3.6+
Flags: blocking-firefox3.6?
Flags: blocking-firefox3.6-
not actively working on this atm, we can mitigate the issue making Places more performant, still revising migration component to be more "modular" could help a lot.
Assignee: mak77 → nobody
Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
Summary: "Script is busy" warning from places flush script on initial migration → "Script is busy" ("Unresponsive Script") warning from places flush script (nsPlacesDBFlush.js) on initial migration / import
This does not block Firefox 4.0, and will be fixed with bug 552023.
blocking2.0: ? → -
Depends on: 552023
Whiteboard: [TSnappiness][status is comment 21][see Bug 492410] → [TSnappiness][status is comment 21][see Bug 492410][fixed-in-places]
this specific bug cannot happen anymore since PlacesDBFlush was just removed
Status: NEW → RESOLVED
Closed: 11 years ago
OS: Windows Vista → All
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b9
Flags: in-litmus?
You need to log in before you can comment on or make changes to this bug.