Closed Bug 620789 Opened 14 years ago Closed 11 years ago

Intermittent browser_bug581253.js | Test timed out followed by Found a tab after previous test timed out

Categories

(Firefox :: Bookmarks & History, defect)

Product:
Firefox
Component:
Bookmarks & History
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 4.0b12

People

(Reporter: philor, Assigned: hiro)

References

Details

(Keywords: intermittent-failure)

Attachments

(5 files, 12 obsolete files)

Use inline function (checked-in)
871 bytes, patch
mak
: review+
Details | Diff | Splinter Review
Check PlacesStarButton._ignoreClicks instead of starButton.hidden (checked-in)
787 bytes, patch
mak
: review+
Details | Diff | Splinter Review
Just clear timeout. (Checked-in)
1.75 KB, patch
mak
: review+
Details | Diff | Splinter Review
review in form of patch [Checked in: Comment 465]
927 bytes, patch
mak
: review+
dietrich
: approval2.0+
Details | Diff | Splinter Review
Proof of that scenario v2
1.71 KB, patch
Details | Diff | Splinter Review 
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1292967961.1292969019.4459.gz#err5
Rev3 Fedora 12x64 mozilla-central opt test mochitest-other on 2010/12/21 13:46:01
s: talos-r3-fed64-029

TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/browser_bug581253.js | Waiting for star button change.
(x a million)
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_bug581253.js | Test timed out
(screenshot)
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_bug581253.js | Found a tab after previous test timed out: data:text/plain,nothing%20but%20plain%20text
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_410196_paste_into_tags.js | node uri's are the same - Got http://example.com/, expected http://mozilla.com/
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_bug581253.js | tags field value was set
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_bug581253.js | the bookmark for the test url has been removed

Not timing out would be good, too, but using registerCleanupFunction() to not mess with other tests running after you were supposed to stop would be a great first step.
Attached patch Part of fix for bug 630789 (obsolete) — — Splinter Review 
I totally agree with Phil's opinion. We should do first clean up browser_bug581253.js state before the next test run.
Attachment #504333 - Flags: review?
Attachment #504333 - Flags: review? → review?(mak77)
Comment on attachment 504333 [details] [diff] [review]
Part of fix for bug 630789

So, I'm fine with doing better cleanup while we figure out the cause, but this patch won't make anything useful as it is, since some of next tests are failing on bookmarks, and this patch is removing history.

You have to add a PlacesUtils.removeFolderChildren(PlacesUtils.unfiledBookmarksFolderId);
to the cleanup function.
So finally this patch will only fix browser_410196_paste_into_tags.js when this one timeouts, nothing more.

Regarding the orange, looks like we never get the popupshown event, either the click is dismissed, something stops propagation or the popup does not open for any other reason (a focus change?).

Notice that in each case, I don't understand the relation between this failure and the globalwarnings or addons tests failures, ideally there is none.
Looks like there could be some issue with arrow panels? all of the failing tests seem to try to show a arrow panel and they are failing doing so.
Attachment #504333 - Flags: review?(mak77) → review-
Attached patch Revised patch (obsolete) — — Splinter Review 
Additional call of removeFolderChildren(PlacesUtils.unfiledBookmarksFolderId).
Attachment #504333 - Attachment is obsolete: true
Attachment #504582 - Flags: review?(mak77)
(In reply to comment #86)

> So, I'm fine with doing better cleanup while we figure out the cause, but this
> patch won't make anything useful as it is, since some of next tests are failing
> on bookmarks, and this patch is removing history.
> 
> You have to add a
> PlacesUtils.removeFolderChildren(PlacesUtils.unfiledBookmarksFolderId);
> to the cleanup function.

Thank you for your keen eyes! I added removeFolderChildren in the new patch.

> So finally this patch will only fix browser_410196_paste_into_tags.js when this
> one timeouts, nothing more.
>
> Regarding the orange, looks like we never get the popupshown event, either the
> click is dismissed, something stops propagation or the popup does not open for
> any other reason (a focus change?).

Yes. I am suspecting bug 624097 and bug 551540 are related to be this issue, but those bugs are only on Linux. But anyway, I could confirm those bugs on Linux, (Unfortunately I can not reproduce the timeout of this issue though.) and suspect there are some focus manager issues on Linux.
Comment on attachment 504582 [details] [diff] [review]
Revised patch

>diff --git a/browser/base/content/test/browser_bug581253.js b/browser/base/content/test/browser_bug581253.js

>+function waitForClearHistoryAndBookmarks()
> {
>+  // Though this function is not in test itself,
>+  // we have to wait for cleaning up history so
>+  // we need to use waitForExplicitFinish here.
>+  waitForExplicitFinish();

I don't know which effect has calling waitForExplicitFinish inside a method invoked by registerCleanupFunction, at first glance it seems that it could be working though since we check for .done before really moving on, but please double check that we don't move to the next test till this is really finished with some debug print or wahtever method you prefer.
Attachment #504582 - Flags: review?(mak77) → review+
Keywords: checkin-needed
Comment on attachment 504582 [details] [diff] [review]
Revised patch

(In reply to comment #108)
> Comment on attachment 504582 [details] [diff] [review]
> Revised patch
> 
> >diff --git a/browser/base/content/test/browser_bug581253.js b/browser/base/content/test/browser_bug581253.js
> 
> >+function waitForClearHistoryAndBookmarks()
> > {
> >+  // Though this function is not in test itself,
> >+  // we have to wait for cleaning up history so
> >+  // we need to use waitForExplicitFinish here.
> >+  waitForExplicitFinish();
> 
> I don't know which effect has calling waitForExplicitFinish inside a method
> invoked by registerCleanupFunction, at first glance it seems that it could be
> working though since we check for .done before really moving on, but please
> double check that we don't move to the next test till this is really finished
> with some debug print or wahtever method you prefer.

Oh, well, actually this is not safe at all.  Cleanup functions are called after SimpleTest.finish: <http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/browser-test.js#162>.
Attachment #504582 - Flags: review+ → review-
Keywords: checkin-needed
(In reply to comment #171)
> Comment on attachment 504582 [details] [diff] [review]
> Revised patch

> Oh, well, actually this is not safe at all.  Cleanup functions are called after
> SimpleTest.finish:
> <http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/browser-test.js#162>.

Ok, I think it could workstill, BUT, I agree that this would be an abuse of the harness dangerous if someone does changes to the harness itself.

Since the scope of the patch so far is trying to remove browser/browser_410196_paste_into_tags.js from the failures list, I'd then suggest to just add the removeFolderChildren call to the beginning of that test as a sanity cleanup.
hm even better, I don't see why we could not just register a cleanup function cleaning only bookmarks. r=me on that!
I am sorry for taking your times, I did not read the codes about registerCleanupFunction well.

(In reply to comment #173)
> hm even better, I don't see why we could not just register a cleanup function
> cleaning only bookmarks. r=me on that!

I agree with you for now. I will attach revised patch. Thank you!
Addressed Marco's comment.
Attachment #507338 - Flags: review?(mak77)
Ooops, I forgot to obsolete the previous patch.. Where can I do that?
(In reply to comment #176)
> Ooops, I forgot to obsolete the previous patch.. Where can I do that?

Details / Edit / Obsolete
Attachment #504582 - Attachment is obsolete: true
(In reply to comment #177)
> (In reply to comment #176)
> > Ooops, I forgot to obsolete the previous patch.. Where can I do that?
> 
> Details / Edit / Obsolete

Thank you!
Comment on attachment 507338 [details] [diff] [review]
Remove bookmarks in cleanupFunction

I'd prefer an inline function since it's really small and avoids the reader to go down looking for what it does.
Btw, both ways the approach is safe, history won't be cleared but here we are just trying to fix the blame, removing a wrongly reported failure.
Attachment #507338 - Flags: review?(mak77) → review+
Used a inline function. Looks better!
Attachment #507338 - Attachment is obsolete: true
Attachment #507345 - Flags: review?(mak77)
Attachment #507345 - Flags: review?(mak77) → review+
Keywords: checkin-needed
Comment on attachment 507345 [details] [diff] [review]
Use inline function (checked-in)

http://hg.mozilla.org/mozilla-central/rev/c07c9b91ec4d
Attachment #507345 - Attachment description: Use inline function → Use inline function (checked-in)
Keywords: checkin-needed
Assignee: nobody → hiikezoe
The havoc, it's gone :)
Summary: Intermittent browser_bug581253.js | Test timed out followed by havoc in other tests → Intermittent browser_bug581253.js | Test timed out followed by Found a tab after previous test timed out
I noticed the following line is included in the last 3 logs (I did not check others):

TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/browser_bug581253.js | Console message: [JavaScript Warning: "WARN addons.manager: Exception calling callback: ReferenceError: PREF_GETADDONS_CACHE_ENABLED is not defined" {file: "chrome://mozapps/content/extensions/extensions.js" line: 1657}]

But bug581253.js does not invoke addons window, so I suspect other test cases before bug581253.js breaks something related to addons. Then I found bug 631844.

I am not sure that fix for bug 631844 will fix this issue though.
Target Milestone: --- → Firefox 4.0b11
I could reproduce this on local Windows XP and found PlacesStarButton._ignoreClicks was still false at http://hg.mozilla.org/mozilla-central/file/e0b266ea525f/browser/base/content/browser-places.js#l1018 when star button is clicked.

And I found this changeset http://hg.mozilla.org/mozilla-central/rev/5d7d04e98fa9

Yay!
Attachment #510244 - Flags: review?(mak77)
Comment on attachment 510244 [details] [diff] [review]
Check PlacesStarButton._ignoreClicks instead of starButton.hidden (checked-in)

Does this fix the failure locally for you? I'm not sure it will be enough, since the failure started days before that changeset, but it's worth taking the change regardless.
Attachment #510244 - Flags: review?(mak77) → review+
Keywords: checkin-needed
Whiteboard: [orange] → [orange][checkin patch in comment 308]
(In reply to comment #309)
> Comment on attachment 510244 [details] [diff] [review]
> Check PlacesStarButton._ignoreClicks instead of starButton.hidden
> 
> Does this fix the failure locally for you? I'm not sure it will be enough,
> since the failure started days before that changeset, but it's worth taking the
> change regardless.

Yes at least on Windows XP, but no on Linux unfortunately. There is another issue about popup window on Linux, I think those three failures before that changeset were caused by this another issue since all three failures were on Linux. Please see bug 609921 and bug 624097 comment 101 - 103.
Comment on attachment 510244 [details] [diff] [review]
Check PlacesStarButton._ignoreClicks instead of starButton.hidden (checked-in)

http://hg.mozilla.org/mozilla-central/rev/94d61f41b3b5
Attachment #510244 - Attachment description: Check PlacesStarButton._ignoreClicks instead of starButton.hidden → Check PlacesStarButton._ignoreClicks instead of starButton.hidden (checked-in)
I'm leaving the bug open due to the rationale in comment 310.
Keywords: checkin-needed
Whiteboard: [orange][checkin patch in comment 308] → [orange]
Note that comment 317 takes us back to the "havoc in other tests" days - the browser_sort_in_library.js failure has browser_bug581253.js stuff mixed in with it.
(In reply to comment #318)
> Note that comment 317 takes us back to the "havoc in other tests" days - the
> browser_sort_in_library.js failure has browser_bug581253.js stuff mixed in with
> it.

Gosh! That havoc is my fault. I forgot to cleaning up timeout function. I will attach a patch soon. And Macro is right, there are some other issues even if on Windows...
either _ignoreClicks or the attribute (or both) are out of sync here...
Attached patch follow up fix (obsolete) — — Splinter Review 
Clear timeout in CleanupFunction if the timeout is still alive, and use requestLongerTimeout since the logs in comment 316 and 317 told us that changing star icon was too late. (It was done in other tests. So it caused havoc in those tests.)

I have no idea about the failures in comment 314 and 315 yet. I need to investigate further detail.
Attachment #510472 - Flags: review?(mak77)
Comment on attachment 510472 [details] [diff] [review]
follow up fix

We don't want to requestLongerTimeout here, the default timeout of 30 seconds is more than enough, the users would be pretty sad if updating the star would take so long.
The test comes back to life later just because the other test is loading a page, and the star updates (as it should do!).

Here there is clearly a problem where _ignoreClicks remains true even if onLocationChange must have been called, I guess if the fact this is a data: uri means something.
Attachment #510472 - Flags: review?(mak77) → review-
Marco, thank you for your review, and please review this again.

> Comment on attachment 510472 [details] [diff] [review]
> follow up fix
> 
> We don't want to requestLongerTimeout here, the default timeout of 30 seconds
> is more than enough, the users would be pretty sad if updating the star would
> take so long.
> The test comes back to life later just because the other test is loading a
> page, and the star updates (as it should do!).

Oh, I see. You are right. I misunderstood the havoc. browser_sidebarpanels.js which is a test case just before those other tests did insert bookmark so the timeout function reacted its updating.

> Here there is clearly a problem where _ignoreClicks remains true even if
> onLocationChange must have been called, I guess if the fact this is a data: uri
> means something.

Maybe, _ignoreClicks became false but "starred" attribute was null just before browser_sidebarpanels_click.js inserted a bookmark.
Attachment #510472 - Attachment is obsolete: true
Attachment #510492 - Flags: review?(mak77)
(In reply to comment #323)
> Maybe, _ignoreClicks became false but "starred" attribute was null just before
> browser_sidebarpanels_click.js inserted a bookmark.

it's hard to tell, but before your change the waitForStar was never waiting, since the first condition was always true, the attribute must have been correct. Changing the first condition made the polling start, so to me the problem seems in _ignoreClicks. I've looked into it, but I can't see a code path that could cause that.
Attachment #510492 - Flags: review?(mak77) → review+
Keywords: checkin-needed
Whiteboard: [orange] → [orange][checkin-needed patch in comment 323, leave bug open please]
http://hg.mozilla.org/mozilla-central/rev/25fd6de1de0d
Keywords: checkin-needed
Whiteboard: [orange][checkin-needed patch in comment 323, leave bug open please] → [orange]
I finally got a failure log with some debug infomation like this:

in updateState: function PSB_updateState() in browser/base/content/browser-places.js 

dump("asyncGetBookmarkIds\n");
    PlacesUtils.asyncGetBookmarkIds(this._uri, function (aItemIds) {
dump("in asyncGetBookmarkIds\n");
      this._itemIds = aItemIds;
      this._updateStateInternal();

      // Start observing bookmarks if needed.
      if (!this._hasBookmarksObserver) {
        try {
          PlacesUtils.bookmarks.addObserver(this, false);
          this._hasBookmarksObserver = true;
        } catch(ex) {
          Components.utils.reportError("PlacesStarButton failed adding a bookmarks observer: " + ex);
        }
      }

dump("set ignore clicks to false);
      // Finally re-enable the star.
      this._ignoreClicks = false;
    }, this);
  },


The log is: 

asyncGetBookmarkIds
TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/browser_bug581253.js | the test url is bookmarked
TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/browser_bug581253.js | Waiting for star button change.true
TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/browser_bug581253.js | Waiting for star button change.true
in asyncGetBookmarkIds
set ignore clicks to false
in asyncGetBookmarkIds
set ignore clicks to false
in asyncGetBookmarkIds
set ignore clicks to false
in asyncGetBookmarkIds
set ignore clicks to false
in asyncGetBookmarkIds
set ignore clicks to false
TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/browser_bug581253.js | Console message: [JavaScript Warning: "WARN addons.manager: Exception calling callback: TypeError: PREF_GETADDONS_CACHE_ENABLED is undefined" {file: "chrome://mozapps/content/extensions/extensions.js" line: 1657}]
TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/browser_bug581253.js | Waiting for star button change.false

The callback function of asyncGetBookmarkIds was invoked 5 times! And _ignoreCLicks seems to be set false correctly. So I think this is a bug of places or storage. I am going to investigate in those modules.
(In reply to comment #358)

> The callback function of asyncGetBookmarkIds was invoked 5 times! And
> _ignoreCLicks seems to be set false correctly. So I think this is a bug of
> places or storage. I am going to investigate in those modules.

Those callbacks may be garbage of other tests.
The real problem is that many statements in asyncGetBookmarkIds had not been executed before bug581253.js runs. So if all of those statements have not been executed while running bug581253.js, timeout occurs. 

I hope this patch will fix this issue completely.
Attachment #511652 - Flags: review?(mak77)
Reinitialize this._asyncPendingStmt with null when the statement is completed.
Attachment #511652 - Attachment is obsolete: true
Attachment #511665 - Flags: review?(mak77)
Attachment #511652 - Flags: review?(mak77)
The previous patch was supposed that asyncGetBookmarkIds is invoked only from updateState. That was wrong. I am sorry for my dumb head. ;-)
Attachment #511665 - Attachment is obsolete: true
Attachment #511686 - Flags: review?(mak77)
Attachment #511665 - Flags: review?(mak77)
hm, while this looks like a really good change, our problem is not that _ignoreClicks is set to false, but that it is wrongly set to true (indeed in our test we wait till the button stops ignoring click, so until it's true).
What could happen is that the first times we wait for _ignoreClicks, when the first spurious callback happens _ignoreClicks is set to false but we wait for "starred" since a previous page could not be starred.
What I don't understand yet, is that when the correct update (the last one) arrives (And they are serialized so it cannot arrive earlier), we should have both _ignoreClicks false and "starred" true, at this point the test should proceed, instead it still timeouts.

So, I think you found a pretty valid and serious bug, but maybe not yet the reason of the timeout, but it's absolutely worth fixing it. Please, file a new bug under Places with your patch and ask me review, a couple changes I'd like to see in that patch:
- instead of this._pendingStmt = null; do a delete this._pendingStmt;
- add a second argument to the AsyncGetBookmarkIds callback that passes out the URI those itemIds are referring to (so it will be invoked like callback(ItemIds, URI))
- as an added safety layer in the StarUI invokation check if the URI you get back from the callback equals this._uri

Thank you for your investigations here!
oh and please fix the javadoc of asyncGetBookmarkIds to both say about the new argument of the callback and add a @note to say that the callback is not invoked if the statement is canceled or hits a error. plus add a @return since now you return something.
hm and actually since we listen for onITemAdded, it's even possible your fix works for this timeout. what I'm thinking is that we get a onItemAdded with information from the wrong bookmarks... If so, nice work!
Attachment #511686 - Flags: review?(mak77) → feedback+
Depends on: 633638
Filed bug 633638.

(In reply to comment #369)
> hm and actually since we listen for onITemAdded, it's even possible your fix
> works for this timeout. what I'm thinking is that we get a onItemAdded with
> information from the wrong bookmarks... If so, nice work!

I do not know exactly what was going on there at that failure time because there were limited information in log, but I think onItemAdded was invoked before waiting star change since the listener had been registered before this test.

So I just think the last request for our URL("data:text/plain,nothing but plain text") had not never processed while running browser_bug581253.js, but I am not sure so there might be (an) other issue(s) to fail this test.
(In reply to comment #367)
> What I don't understand yet, is that when the correct update (the last one)
> arrives (And they are serialized so it cannot arrive earlier), we should have
> both _ignoreClicks false and "starred" true, at this point the test should
> proceed, instead it still timeouts.

You are absolutely right. I still got a failure on my local WindowsXP even if the patch for 633638 is applied. Unfortunately I did remove all debug codes at that failure time, so I could not any hints to track the failure reason. Anyway, there are some other issues to be failed browser_bug581253.js. I go on investigating this issue.
Unfortunately bug 633638 won't fix this yet (I got still failures on try).

I've pushed a small info() dump to try to figure out this failure, or at least have more data:
http://hg.mozilla.org/mozilla-central/rev/03b1f86f730b
So, the debug code I added said something, we are stuck in this state.

pendingStmt: false, hasAttribute: false, tracked uri: data:text/plain,nothing%20but%20plain%20text

There is no pending statement (probably fine), the tracked uri is correct, but the "starred" attribute is missing.
exactly we move from
pendingStmt: true, hasAttribute: true, tracked uri: data:text/plain,nothing%20but%20plain%20text
to
pendingStmt: false, hasAttribute: false, tracked uri: data:text/plain,nothing%20but%20plain%20text
Attachment #511686 - Attachment is obsolete: true
Attachment #510492 - Attachment description: Just clear timeout. → Just clear timeout. (Checked-in)
(In reply to comment #422)
> exactly we move from
> pendingStmt: true, hasAttribute: true, tracked uri:
> data:text/plain,nothing%20but%20plain%20text

At this time, I guess "starred" attribute had been set in onItemAdded for the tracked uri but still the pendingStmt had been in progress or in queue.

> pendingStmt: false, hasAttribute: false, tracked uri:
> data:text/plain,nothing%20but%20plain%20text

This is a real problem for us. Who did remove the "starred" attribute?
If the pendingStmt works well, the attribute is set correctly. 

So there are two possible cases.

1. asyncGetBookmarkIds does no return zero length aItemIds. 
2. Someone remove the attribute after the attribute is set in the callback of PlacesUtils.asyncGetBookmarkIds.

weird...
I got a failure on local windows XP. asyncGetBookmarkIds seems to return empty aItemIds at that time.

I changed browser-places.js like this:
--- a/browser/base/content/browser-places.js
+++ b/browser/base/content/browser-places.js
@@ -1015,16 +1015,17 @@ var PlacesStarButton = {
       return;
     }
 
     if (this._itemIds.length > 0) {
       this._starIcon.setAttribute("starred", "true");
       this._starIcon.setAttribute("tooltiptext", this._starredTooltip);
     }
     else {
+    dump("remove starred attribute: " + arguments.callee.caller + "\n");
       this._starIcon.removeAttribute("starred");
       this._starIcon.setAttribute("tooltiptext", this._unstarredTooltip);
     }
   },
 
   onClick: function PSB_onClick(aEvent)
   {
     // Ignore clicks on the star while we update its state.



And the log is here:

TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/browser_bug581253.js | pendingStmt: true, hasAttribute: true, tracked uri: data:text/plain,nothing%20but%20plain%20text^M
remove starred attribute: function (aItemIds, aURI) {^M
    if (!aURI.equals(this._uri)) {^M
        Components.utils.reportError("PlacesStarButton did not receive current URI");^M
        return;^M
    }^M
    this._itemIds = aItemIds;^M
    this._updateStateInternal();^M
    if (!this._hasBookmarksObserver) {^M
        try {^M
            PlacesUtils.bookmarks.addObserver(this, false);^M
            this._hasBookmarksObserver = true;^M
        } catch (ex) {^M
            Components.utils.reportError("PlacesStarButton failed adding a bookmarks observer: " + ex);^M
        }^M
    }^M
    delete this._pendingStmt;^M
}^M
Gosh!!!!!!!!!! I must be blind!

I really hope this is the final fix!
Attachment #513049 - Flags: review?(mak77)
Comment on attachment 513049 [details] [diff] [review]
Get the first row in the result set!

Ooops! I got the wrong idea.
Attachment #513049 - Flags: review?(mak77)
Attachment #513049 - Attachment is obsolete: true
Attached patch Possible fix (obsolete) — — Splinter Review 
I guess this issue is caused by priority order of onItemAdded and 
mozIStorageBaseStatement.executeAsync callback.

My assumption of the failure scenario is:

1. onLocationChange event
  PlacesUtils.asyncGetBookmarkIds was invoked.
2. The result set had been prepared, but handleCompletion (maybe and handleResult) had not been invoked yet.
  So the number of the result records is 0 since the URI had not been bookmarked yet. 
3. onLoadEvent
  the URI had been bookmared.
4. onItemAdded event
  "starred" attribute was set.
5. handleComletion has been invoked with 0 records.
  then "starred" attribute was removed.
Attachment #513074 - Flags: review?(mak77)
Comment on attachment 513074 [details] [diff] [review]
Possible fix

Oopps, This patch does not work properly.
Attachment #513074 - Flags: review?(mak77)
I hope I am not wrong this time.
Attachment #513079 - Flags: review?(mak77)
Hm, I like the hypothesis but not the patch, since it's not going to work correctly for a uri bookmarked multiple times (you don't store aItemIds), so this._itemIds could go out of sync.
What we should do is empty this._itemIds when we set this._uri (to remove old entries) and when we get aItemIds concat it with this._itemIds.
The only remaining issue would be duplicates detection. We could do something like this._itemIds = this._itemIds.filter(dupe_detection).concat(aItemIds)
This way we both preserve the edge case of a bookmark added onload and other bookmarks read from the db.
Attachment #513079 - Flags: review?(mak77) → review-
(In reply to comment #444)
> Hm, I like the hypothesis but not the patch, since it's not going to work
> correctly for a uri bookmarked multiple times (you don't store aItemIds), so
> this._itemIds could go out of sync.
> What we should do is empty this._itemIds when we set this._uri (to remove old
> entries) and when we get aItemIds concat it with this._itemIds.
> The only remaining issue would be duplicates detection. We could do something
> like this._itemIds = this._itemIds.filter(dupe_detection).concat(aItemIds)

You mean this._itemIds.concat(aItemIds).filter(dupe_detection)?
well, for dupe_detection I meant "retain only ids that are NOT in aItemIds", sorry for the wrong choice of words
Attached patch Revised patch (obsolete) — — Splinter Review 
If this patch has still any problems (need comment or something I am wrong!), please fix as you wish. I am going to sleep. Thank you.
Attachment #513074 - Attachment is obsolete: true
Attachment #513079 - Attachment is obsolete: true
Attachment #513097 - Flags: review?(mak77)
Thank you!
This is my review in form of patch.

The fix is safe enough for asking approval, covers a edge case where a bookmark is added onload.
Attachment #513097 - Attachment is obsolete: true
Attachment #513109 - Flags: review+
Attachment #513109 - Flags: approval2.0?
Attachment #513097 - Flags: review?(mak77)
Comment on attachment 513109 [details] [diff] [review]
review in form of patch
[Checked in: Comment 465]

a=me for orange fix.
Attachment #513109 - Flags: approval2.0? → approval2.0+
Attached patch Proof of that scenario (obsolete) — — Splinter Review 
Marco, thank you for your great review and advices!

For the record, I created a test case to confirm my assumption happens surely.

Of course this test case is not valid because do_check_eq in the callback of the second asyncGetBookmarkIds might be fail (always fails on my local Linux at least).
Comment on attachment 513294 [details] [diff] [review]
Proof of that scenario

Oh my dumb head. This test is wrong.
Attachment #513294 - Attachment is obsolete: true
This is it.
Keywords: checkin-needed
tentative closing:
http://hg.mozilla.org/mozilla-central/rev/4d19c1a15f69
Status: NEW → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: Firefox 4.0b11 → Firefox 4.0b12
Attachment #513109 - Attachment description: review in form of patch → review in form of patch [Checked in: Comment 465]
Whiteboard: [orange]
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
May I suggest to file a new (blocking or dependent) bug, as this one was resolved "fixed" two years ago ?
Status: REOPENED → RESOLVED
Closed: 13 years ago → 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: