From c09c84b7eb1c4a64a2baf798d5dfae068047ae6e Mon Sep 17 00:00:00 2001 From: Masayuki Nakano Date: Thu, 15 Sep 2016 22:36:23 +0900 Subject: [PATCH 01/29] Bug 1138159 Don't reset IM context at selection change when there is no composition and hasn't retrieved surrounding text after last selection change r=m_kato ibus-pinyin has a bug. When application calls gtk_im_context_reset(), which means selection is changed in application, ibus-pinyin sents a set of composition signals with empty commit string. Therefore, selecting text causes removing it. For preventing it but not breaking the other IMEs which use surrounding text, we should give up to call gtk_im_context_reset() if IME hasn't retrieved surrounding text after the last selection change. Not having retrieved surrounding text means that the IME doesn't have any cache of contents. Therefore, not calling gtk_im_context_reset() at selection change must be safe for such IMEs. MozReview-Commit-ID: 5cbIZjpd7zN --HG-- extra : rebase_source : 6010b3e055d66ebd2ed50f9b3ee8ff2330d3c6ab --- widget/gtk/IMContextWrapper.cpp | 24 +++++++++++++++++++++--- widget/gtk/IMContextWrapper.h | 3 +++ 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/widget/gtk/IMContextWrapper.cpp b/widget/gtk/IMContextWrapper.cpp index a90f52711ae3f..34dbbbd1cc025 100644 --- a/widget/gtk/IMContextWrapper.cpp +++ b/widget/gtk/IMContextWrapper.cpp @@ -179,6 +179,7 @@ IMContextWrapper::IMContextWrapper(nsWindow* aOwnerWindow) , mLayoutChanged(false) , mSetCursorPositionOnKeyEvent(true) , mPendingResettingIMContext(false) + , mRetrieveSurroundingSignalReceived(false) { static bool sFirstInstance = true; if (sFirstInstance) { @@ -930,6 +931,9 @@ IMContextWrapper::OnSelectionChange(nsWindow* aCaller, const IMENotification& aIMENotification) { mSelection.Assign(aIMENotification); + bool retrievedSurroundingSignalReceived = + mRetrieveSurroundingSignalReceived; + mRetrieveSurroundingSignalReceived = false; if (MOZ_UNLIKELY(IsDestroyed())) { return; @@ -943,7 +947,8 @@ IMContextWrapper::OnSelectionChange(nsWindow* aCaller, "mSelectionChangeData={ mOffset=%u, Length()=%u, mReversed=%s, " "mWritingMode=%s, mCausedByComposition=%s, " "mCausedBySelectionEvent=%s, mOccurredDuringComposition=%s " - "} }), mCompositionState=%s, mIsDeletingSurrounding=%s", + "} }), mCompositionState=%s, mIsDeletingSurrounding=%s, " + "mRetrieveSurroundingSignalReceived=%s", this, aCaller, selectionChangeData.mOffset, selectionChangeData.Length(), ToChar(selectionChangeData.mReversed), @@ -951,7 +956,8 @@ IMContextWrapper::OnSelectionChange(nsWindow* aCaller, ToChar(selectionChangeData.mCausedByComposition), ToChar(selectionChangeData.mCausedBySelectionEvent), ToChar(selectionChangeData.mOccurredDuringComposition), - GetCompositionStateName(), ToChar(mIsDeletingSurrounding))); + GetCompositionStateName(), ToChar(mIsDeletingSurrounding), + ToChar(retrievedSurroundingSignalReceived))); if (aCaller != mLastFocusedWindow) { MOZ_LOG(gGtkIMLog, LogLevel::Error, @@ -1011,7 +1017,18 @@ IMContextWrapper::OnSelectionChange(nsWindow* aCaller, if (!selectionChangeData.mCausedByComposition && !selectionChangeData.mCausedBySelectionEvent && !occurredBeforeComposition) { - ResetIME(); + // Hack for ibus-pinyin. ibus-pinyin will synthesize a set of + // composition which commits with empty string after calling + // gtk_im_context_reset(). Therefore, selecting text causes + // unexpectedly removing it. For preventing it but not breaking the + // other IMEs which use surrounding text, we should call it only when + // surrounding text has been retrieved after last selection range was + // set. If it's not retrieved, that means that current IME doesn't + // have any content cache, so, it must not need the notification of + // selection change. + if (IsComposing() || retrievedSurroundingSignalReceived) { + ResetIME(); + } } } @@ -1164,6 +1181,7 @@ IMContextWrapper::OnRetrieveSurroundingNative(GtkIMContext* aContext) AppendUTF16toUTF8(nsDependentSubstring(uniStr, cursorPos), utf8Str); gtk_im_context_set_surrounding(aContext, utf8Str.get(), utf8Str.Length(), cursorPosInUTF8); + mRetrieveSurroundingSignalReceived = true; return TRUE; } diff --git a/widget/gtk/IMContextWrapper.h b/widget/gtk/IMContextWrapper.h index ae60c54643fb0..e869c160a86fe 100644 --- a/widget/gtk/IMContextWrapper.h +++ b/widget/gtk/IMContextWrapper.h @@ -284,6 +284,9 @@ class IMContextWrapper final : public TextEventDispatcherListener // the composition in such case. However, we should notify IME of the // selection change after the composition is committed. bool mPendingResettingIMContext; + // mRetrieveSurroundingSignalReceived is true after "retrieve_surrounding" + // signal is received until selection is changed in Gecko. + bool mRetrieveSurroundingSignalReceived; // sLastFocusedContext is a pointer to the last focused instance of this // class. When a instance is destroyed and sLastFocusedContext refers it, From 7b00b004c406274b154c8e26458c1eec5defc9aa Mon Sep 17 00:00:00 2001 From: Mark Hammond Date: Tue, 5 Jul 2016 16:02:19 +1000 Subject: [PATCH 02/29] Bug 1276823 - ensure bookmarks engine tracks parents when de-duping incoming bookmarks. r=rnewman MozReview-Commit-ID: ym58KTB89b --HG-- extra : rebase_source : 2090299a4ce68fac2979f85e5886c3c5e10c61c3 --- services/sync/modules/engines.js | 46 +- services/sync/modules/engines/bookmarks.js | 53 +- .../sync/tests/unit/test_bookmark_duping.js | 644 ++++++++++++++++++ services/sync/tests/unit/xpcshell.ini | 1 + 4 files changed, 724 insertions(+), 20 deletions(-) create mode 100644 services/sync/tests/unit/test_bookmark_duping.js diff --git a/services/sync/modules/engines.js b/services/sync/modules/engines.js index 83b3d63f463a2..93dc0274379f5 100644 --- a/services/sync/modules/engines.js +++ b/services/sync/modules/engines.js @@ -150,7 +150,7 @@ Tracker.prototype = { // Default to the current time in seconds if no time is provided. if (when == null) { - when = Date.now() / 1000; + when = this._now(); } // Add/update the entry if we have a newer time. @@ -183,6 +183,10 @@ Tracker.prototype = { this.saveChangedIDs(); }, + _now() { + return Date.now() / 1000; + }, + _isTracking: false, // Override these in your subclasses. @@ -1275,6 +1279,18 @@ SyncEngine.prototype = { this._delete.ids.push(id); }, + _switchItemToDupe(localDupeGUID, incomingItem) { + // The local, duplicate ID is always deleted on the server. + this._deleteId(localDupeGUID); + + // We unconditionally change the item's ID in case the engine knows of + // an item but doesn't expose it through itemExists. If the API + // contract were stronger, this could be changed. + this._log.debug("Switching local ID to incoming: " + localDupeGUID + " -> " + + incomingItem.id); + this._store.changeItemID(localDupeGUID, incomingItem.id); + }, + /** * Reconcile incoming record with local state. * @@ -1348,40 +1364,32 @@ SyncEngine.prototype = { // refresh the metadata collected above. See bug 710448 for the history // of this logic. if (!existsLocally) { - let dupeID = this._findDupe(item); - if (dupeID) { - this._log.trace("Local item " + dupeID + " is a duplicate for " + + let localDupeGUID = this._findDupe(item); + if (localDupeGUID) { + this._log.trace("Local item " + localDupeGUID + " is a duplicate for " + "incoming item " + item.id); - // The local, duplicate ID is always deleted on the server. - this._deleteId(dupeID); - // The current API contract does not mandate that the ID returned by // _findDupe() actually exists. Therefore, we have to perform this // check. - existsLocally = this._store.itemExists(dupeID); - - // We unconditionally change the item's ID in case the engine knows of - // an item but doesn't expose it through itemExists. If the API - // contract were stronger, this could be changed. - this._log.debug("Switching local ID to incoming: " + dupeID + " -> " + - item.id); - this._store.changeItemID(dupeID, item.id); + existsLocally = this._store.itemExists(localDupeGUID); // If the local item was modified, we carry its metadata forward so // appropriate reconciling can be performed. - if (this._modified.has(dupeID)) { + if (this._modified.has(localDupeGUID)) { locallyModified = true; - localAge = Date.now() / 1000 - - this._modified.getModifiedTimestamp(dupeID); + localAge = this._tracker._now() - this._modified.getModifiedTimestamp(localDupeGUID); remoteIsNewer = remoteAge < localAge; - this._modified.swap(dupeID, item.id); + this._modified.swap(localDupeGUID, item.id); } else { locallyModified = false; localAge = null; } + // Tell the engine to do whatever it needs to switch the items. + this._switchItemToDupe(localDupeGUID, item); + this._log.debug("Local item after duplication: age=" + localAge + "; modified=" + locallyModified + "; exists=" + existsLocally); diff --git a/services/sync/modules/engines/bookmarks.js b/services/sync/modules/engines/bookmarks.js index f27fa0bc3d6ea..2d98bacf6a89e 100644 --- a/services/sync/modules/engines/bookmarks.js +++ b/services/sync/modules/engines/bookmarks.js @@ -509,6 +509,57 @@ BookmarksEngine.prototype = { } return guids; }, + + // Called when _findDupe returns a dupe item and the engine has decided to + // switch the existing item to the new incoming item. + _switchItemToDupe(localDupeGUID, incomingItem) { + // We unconditionally change the item's ID in case the engine knows of + // an item but doesn't expose it through itemExists. If the API + // contract were stronger, this could be changed. + this._log.debug("Switching local ID to incoming: " + localDupeGUID + " -> " + + incomingItem.id); + this._store.changeItemID(localDupeGUID, incomingItem.id); + + // And mark the parent as being modified. Given we de-dupe based on the + // parent *name* it's possible the item having its GUID changed has a + // different parent from the incoming record. + // So we need to find the GUID of the local parent. + let now = this._tracker._now(); + let localID = this._store.idForGUID(incomingItem.id); + let localParentID = PlacesUtils.bookmarks.getFolderIdForItem(localID); + let localParentGUID = this._store.GUIDForId(localParentID); + this._modified.set(localParentGUID, { modified: now, deleted: false }); + + // And we also add the parent as reflected in the incoming record as the + // de-dupe process might have used an existing item in a different folder. + // But only if the parent exists, otherwise we will upload a deleted item + // when it might actually be valid, just unknown to us. Note that this + // scenario will still leave us with inconsistent client and server states; + // the incoming record on the server references a parent that isn't the + // actual parent locally - see bug 1297955. + if (localParentGUID != incomingItem.parentid) { + let remoteParentID = this._store.idForGUID(incomingItem.parentid); + if (remoteParentID > 0) { + // The parent specified in the record does exist, so we are going to + // attempt a move when we come to applying the record. Mark the parent + // as being modified so we will later upload it with the new child + // reference. + this._modified.set(incomingItem.parentid, { modified: now, deleted: false }); + } else { + // We aren't going to do a move as we don't have the parent (yet?). + // When applying the record we will add our special PARENT_ANNO + // annotation, so if it arrives in the future (either this Sync or a + // later one) it will be reparented. + this._log.debug(`Incoming duplicate item ${incomingItem.id} specifies ` + + `non-existing parent ${incomingItem.parentid}`); + } + } + + // The local, duplicate ID is always deleted on the server - but for + // bookmarks it is a logical delete. + // Simply adding this (now non-existing) ID to the tracker is enough. + this._modified.set(localDupeGUID, { modified: now, deleted: true }); + }, }; function BookmarksStore(name, engine) { @@ -565,7 +616,7 @@ BookmarksStore.prototype = { if (!parentGUID) { throw "Record " + record.id + " has invalid parentid: " + parentGUID; } - this._log.debug("Local parent is " + parentGUID); + this._log.debug("Remote parent is " + parentGUID); // Do the normal processing of incoming records Store.prototype.applyIncoming.call(this, record); diff --git a/services/sync/tests/unit/test_bookmark_duping.js b/services/sync/tests/unit/test_bookmark_duping.js new file mode 100644 index 0000000000000..42551b6ff7642 --- /dev/null +++ b/services/sync/tests/unit/test_bookmark_duping.js @@ -0,0 +1,644 @@ +/* Any copyright is dedicated to the Public Domain. + http://creativecommons.org/publicdomain/zero/1.0/ */ + +Cu.import("resource://gre/modules/PlacesUtils.jsm"); +Cu.import("resource://services-common/async.js"); +Cu.import("resource://gre/modules/Log.jsm"); +Cu.import("resource://services-sync/engines.js"); +Cu.import("resource://services-sync/engines/bookmarks.js"); +Cu.import("resource://services-sync/service.js"); +Cu.import("resource://services-sync/util.js"); +Cu.import("resource://testing-common/services/sync/utils.js"); +Cu.import("resource://services-sync/bookmark_validator.js"); + + +initTestLogging("Trace"); + +const bms = PlacesUtils.bookmarks; + +Service.engineManager.register(BookmarksEngine); + +const engine = new BookmarksEngine(Service); +const store = engine._store; +store._log.level = Log.Level.Trace; +engine._log.level = Log.Level.Trace; + +function promiseOneObserver(topic) { + return new Promise((resolve, reject) => { + let observer = function(subject, topic, data) { + Services.obs.removeObserver(observer, topic); + resolve({ subject: subject, data: data }); + } + Services.obs.addObserver(observer, topic, false); + }); +} + +function setup() { + let server = serverForUsers({"foo": "password"}, { + meta: {global: {engines: {bookmarks: {version: engine.version, + syncID: engine.syncID}}}}, + bookmarks: {}, + }); + + generateNewKeys(Service.collectionKeys); + + new SyncTestingInfrastructure(server.server); + + let collection = server.user("foo").collection("bookmarks"); + + Svc.Obs.notify("weave:engine:start-tracking"); // We skip usual startup... + + return { server, collection }; +} + +function* cleanup(server) { + Svc.Obs.notify("weave:engine:stop-tracking"); + Services.prefs.setBoolPref("services.sync-testing.startOverKeepIdentity", true); + let promiseStartOver = promiseOneObserver("weave:service:start-over:finish"); + Service.startOver(); + yield promiseStartOver; + yield new Promise(resolve => server.stop(resolve)); + yield bms.eraseEverything(); +} + +function getFolderChildrenIDs(folderId) { + let index = 0; + let result = []; + while (true) { + let childId = bms.getIdForItemAt(folderId, index); + if (childId == -1) { + break; + } + result.push(childId); + index++; + } + return result; +} + +function createFolder(parentId, title) { + let id = bms.createFolder(parentId, title, 0); + let guid = store.GUIDForId(id); + return { id, guid }; +} + +function createBookmark(parentId, url, title, index = bms.DEFAULT_INDEX) { + let uri = Utils.makeURI(url); + let id = bms.insertBookmark(parentId, uri, index, title) + let guid = store.GUIDForId(id); + return { id, guid }; +} + +function getServerRecord(collection, id) { + let wbo = collection.get({ full: true, ids: [id] }); + // Whew - lots of json strings inside strings. + return JSON.parse(JSON.parse(JSON.parse(wbo).payload).ciphertext); +} + +function* promiseNoLocalItem(guid) { + // Check there's no item with the specified guid. + let got = yield bms.fetch({ guid }); + ok(!got, `No record remains with GUID ${guid}`); + // and while we are here ensure the places cache doesn't still have it. + yield Assert.rejects(PlacesUtils.promiseItemId(guid)); +} + +function* validate(collection, expectedFailures = []) { + let validator = new BookmarkValidator(); + let records = collection.payloads(); + + let problems = validator.inspectServerRecords(records).problemData; + // all non-zero problems. + let summary = problems.getSummary().filter(prob => prob.count != 0); + + // split into 2 arrays - expected and unexpected. + let isInExpectedFailures = elt => { + for (let i = 0; i < expectedFailures.length; i++) { + if (elt.name == expectedFailures[i].name && elt.count == expectedFailures[i].count) { + return true; + } + } + return false; + } + let expected = []; + let unexpected = []; + for (let elt of summary) { + (isInExpectedFailures(elt) ? expected : unexpected).push(elt); + } + if (unexpected.length || expected.length != expectedFailures.length) { + do_print("Validation failed:"); + do_print(JSON.stringify(summary)); + // print the entire validator output as it has IDs etc. + do_print(JSON.stringify(problems, undefined, 2)); + // All server records and the entire bookmark tree. + do_print("Server records:\n" + JSON.stringify(collection.payloads(), undefined, 2)); + let tree = yield PlacesUtils.promiseBookmarksTree("", { includeItemIds: true }); + do_print("Local bookmark tree:\n" + JSON.stringify(tree, undefined, 2)); + ok(false); + } +} + +add_task(function* test_dupe_bookmark() { + _("Ensure that a bookmark we consider a dupe is handled correctly."); + + let { server, collection } = this.setup(); + + try { + // The parent folder and one bookmark in it. + let {id: folder1_id, guid: folder1_guid } = createFolder(bms.toolbarFolder, "Folder 1"); + let {id: bmk1_id, guid: bmk1_guid} = createBookmark(folder1_id, "http://getfirefox.com/", "Get Firefox!"); + + engine.sync(); + + // We've added the bookmark, its parent (folder1) plus "menu", "toolbar" and "unfiled" + equal(collection.count(), 5); + equal(getFolderChildrenIDs(folder1_id).length, 1); + + // Now create a new incoming record that looks alot like a dupe. + let newGUID = Utils.makeGUID(); + let to_apply = { + id: newGUID, + bmkUri: "http://getfirefox.com/", + type: "bookmark", + title: "Get Firefox!", + parentName: "Folder 1", + parentid: folder1_guid, + }; + + collection.insert(newGUID, encryptPayload(to_apply), Date.now() / 1000 + 10); + _("Syncing so new dupe record is processed"); + engine.lastSync = engine.lastSync - 0.01; + engine.sync(); + + // We should have logically deleted the dupe record. + equal(collection.count(), 6); + ok(getServerRecord(collection, bmk1_guid).deleted); + // and physically removed from the local store. + yield promiseNoLocalItem(bmk1_guid); + // Parent should still only have 1 item. + equal(getFolderChildrenIDs(folder1_id).length, 1); + // The parent record on the server should now reference the new GUID and not the old. + let serverRecord = getServerRecord(collection, folder1_guid); + ok(!serverRecord.children.includes(bmk1_guid)); + ok(serverRecord.children.includes(newGUID)); + + // and a final sanity check - use the validator + yield validate(collection); + } finally { + yield cleanup(server); + } +}); + +add_task(function* test_dupe_reparented_bookmark() { + _("Ensure that a bookmark we consider a dupe from a different parent is handled correctly"); + + let { server, collection } = this.setup(); + + try { + // The parent folder and one bookmark in it. + let {id: folder1_id, guid: folder1_guid } = createFolder(bms.toolbarFolder, "Folder 1"); + let {id: bmk1_id, guid: bmk1_guid} = createBookmark(folder1_id, "http://getfirefox.com/", "Get Firefox!"); + // Another parent folder *with the same name* + let {id: folder2_id, guid: folder2_guid } = createFolder(bms.toolbarFolder, "Folder 1"); + + do_print(`folder1_guid=${folder1_guid}, folder2_guid=${folder2_guid}, bmk1_guid=${bmk1_guid}`); + + engine.sync(); + + // We've added the bookmark, 2 folders plus "menu", "toolbar" and "unfiled" + equal(collection.count(), 6); + equal(getFolderChildrenIDs(folder1_id).length, 1); + equal(getFolderChildrenIDs(folder2_id).length, 0); + + // Now create a new incoming record that looks alot like a dupe of the + // item in folder1_guid, but with a record that points to folder2_guid. + let newGUID = Utils.makeGUID(); + let to_apply = { + id: newGUID, + bmkUri: "http://getfirefox.com/", + type: "bookmark", + title: "Get Firefox!", + parentName: "Folder 1", + parentid: folder2_guid, + }; + + collection.insert(newGUID, encryptPayload(to_apply), Date.now() / 1000 + 10); + + _("Syncing so new dupe record is processed"); + engine.lastSync = engine.lastSync - 0.01; + engine.sync(); + + // We should have logically deleted the dupe record. + equal(collection.count(), 7); + ok(getServerRecord(collection, bmk1_guid).deleted); + // and physically removed from the local store. + yield promiseNoLocalItem(bmk1_guid); + // The original folder no longer has the item + equal(getFolderChildrenIDs(folder1_id).length, 0); + // But the second dupe folder does. + equal(getFolderChildrenIDs(folder2_id).length, 1); + + // The record for folder1 on the server should reference neither old or new GUIDs. + let serverRecord1 = getServerRecord(collection, folder1_guid); + ok(!serverRecord1.children.includes(bmk1_guid)); + ok(!serverRecord1.children.includes(newGUID)); + + // The record for folder2 on the server should only reference the new new GUID. + let serverRecord2 = getServerRecord(collection, folder2_guid); + ok(!serverRecord2.children.includes(bmk1_guid)); + ok(serverRecord2.children.includes(newGUID)); + + // and a final sanity check - use the validator + yield validate(collection); + } finally { + yield cleanup(server); + } +}); + +add_task(function* test_dupe_reparented_locally_changed_bookmark() { + _("Ensure that a bookmark with local changes we consider a dupe from a different parent is handled correctly"); + + let { server, collection } = this.setup(); + + try { + // The parent folder and one bookmark in it. + let {id: folder1_id, guid: folder1_guid } = createFolder(bms.toolbarFolder, "Folder 1"); + let {id: bmk1_id, guid: bmk1_guid} = createBookmark(folder1_id, "http://getfirefox.com/", "Get Firefox!"); + // Another parent folder *with the same name* + let {id: folder2_id, guid: folder2_guid } = createFolder(bms.toolbarFolder, "Folder 1"); + + do_print(`folder1_guid=${folder1_guid}, folder2_guid=${folder2_guid}, bmk1_guid=${bmk1_guid}`); + + engine.sync(); + + // We've added the bookmark, 2 folders plus "menu", "toolbar" and "unfiled" + equal(collection.count(), 6); + equal(getFolderChildrenIDs(folder1_id).length, 1); + equal(getFolderChildrenIDs(folder2_id).length, 0); + + // Now create a new incoming record that looks alot like a dupe of the + // item in folder1_guid, but with a record that points to folder2_guid. + let newGUID = Utils.makeGUID(); + let to_apply = { + id: newGUID, + bmkUri: "http://getfirefox.com/", + type: "bookmark", + title: "Get Firefox!", + parentName: "Folder 1", + parentid: folder2_guid, + }; + + collection.insert(newGUID, encryptPayload(to_apply), Date.now() / 1000 + 10); + + // Make a change to the bookmark that's a dupe, and set the modification + // time further in the future than the incoming record. This will cause + // us to issue the infamous "DATA LOSS" warning in the logs but cause us + // to *not* apply the incoming record. + engine._tracker.addChangedID(bmk1_guid, Date.now() / 1000 + 60); + + _("Syncing so new dupe record is processed"); + engine.lastSync = engine.lastSync - 0.01; + engine.sync(); + + // We should have logically deleted the dupe record. + equal(collection.count(), 7); + ok(getServerRecord(collection, bmk1_guid).deleted); + // and physically removed from the local store. + yield promiseNoLocalItem(bmk1_guid); + // The original folder still longer has the item + equal(getFolderChildrenIDs(folder1_id).length, 1); + // The second folder does not. + equal(getFolderChildrenIDs(folder2_id).length, 0); + + // The record for folder1 on the server should reference only the GUID. + let serverRecord1 = getServerRecord(collection, folder1_guid); + ok(!serverRecord1.children.includes(bmk1_guid)); + ok(serverRecord1.children.includes(newGUID)); + + // The record for folder2 on the server should reference nothing. + let serverRecord2 = getServerRecord(collection, folder2_guid); + ok(!serverRecord2.children.includes(bmk1_guid)); + ok(!serverRecord2.children.includes(newGUID)); + + // and a final sanity check - use the validator + yield validate(collection); + } finally { + yield cleanup(server); + } +}); + +add_task(function* test_dupe_reparented_to_earlier_appearing_parent_bookmark() { + _("Ensure that a bookmark we consider a dupe from a different parent that " + + "appears in the same sync before the dupe item"); + + let { server, collection } = this.setup(); + + try { + // The parent folder and one bookmark in it. + let {id: folder1_id, guid: folder1_guid } = createFolder(bms.toolbarFolder, "Folder 1"); + let {id: bmk1_id, guid: bmk1_guid} = createBookmark(folder1_id, "http://getfirefox.com/", "Get Firefox!"); + // One more folder we'll use later. + let {id: folder2_id, guid: folder2_guid} = createFolder(bms.toolbarFolder, "A second folder"); + + do_print(`folder1=${folder1_guid}, bmk1=${bmk1_guid} folder2=${folder2_guid}`); + + engine.sync(); + + // We've added the bookmark, 2 folders plus "menu", "toolbar" and "unfiled" + equal(collection.count(), 6); + equal(getFolderChildrenIDs(folder1_id).length, 1); + + let newGUID = Utils.makeGUID(); + let newParentGUID = Utils.makeGUID(); + + // Have the new parent appear before the dupe item. + collection.insert(newParentGUID, encryptPayload({ + id: newParentGUID, + type: "folder", + title: "Folder 1", + parentName: "A second folder", + parentid: folder2_guid, + children: [newGUID], + tags: [], + }), Date.now() / 1000 + 10); + + // And also the update to "folder 2" that references the new parent. + collection.insert(folder2_guid, encryptPayload({ + id: folder2_guid, + type: "folder", + title: "A second folder", + parentName: "Bookmarks Toolbar", + parentid: "toolbar", + children: [newParentGUID], + tags: [], + }), Date.now() / 1000 + 10); + + // Now create a new incoming record that looks alot like a dupe of the + // item in folder1_guid, with a record that points to a parent with the + // same name which appeared earlier in this sync. + collection.insert(newGUID, encryptPayload({ + id: newGUID, + bmkUri: "http://getfirefox.com/", + type: "bookmark", + title: "Get Firefox!", + parentName: "Folder 1", + parentid: newParentGUID, + tags: [], + }), Date.now() / 1000 + 10); + + + _("Syncing so new records are processed."); + engine.lastSync = engine.lastSync - 0.01; + engine.sync(); + + // Everything should be parented correctly. + equal(getFolderChildrenIDs(folder1_id).length, 0); + let newParentID = store.idForGUID(newParentGUID); + let newID = store.idForGUID(newGUID); + deepEqual(getFolderChildrenIDs(newParentID), [newID]); + + // Make sure the validator thinks everything is hunky-dory. + yield validate(collection); + } finally { + yield cleanup(server); + } +}); + +add_task(function* test_dupe_reparented_to_later_appearing_parent_bookmark() { + _("Ensure that a bookmark we consider a dupe from a different parent that " + + "doesn't exist locally as we process the child, but does appear in the same sync"); + + let { server, collection } = this.setup(); + + try { + // The parent folder and one bookmark in it. + let {id: folder1_id, guid: folder1_guid } = createFolder(bms.toolbarFolder, "Folder 1"); + let {id: bmk1_id, guid: bmk1_guid} = createBookmark(folder1_id, "http://getfirefox.com/", "Get Firefox!"); + // One more folder we'll use later. + let {id: folder2_id, guid: folder2_guid} = createFolder(bms.toolbarFolder, "A second folder"); + + do_print(`folder1=${folder1_guid}, bmk1=${bmk1_guid} folder2=${folder2_guid}`); + + engine.sync(); + + // We've added the bookmark, 2 folders plus "menu", "toolbar" and "unfiled" + equal(collection.count(), 6); + equal(getFolderChildrenIDs(folder1_id).length, 1); + + // Now create a new incoming record that looks alot like a dupe of the + // item in folder1_guid, but with a record that points to a parent with the + // same name, but a non-existing local ID. + let newGUID = Utils.makeGUID(); + let newParentGUID = Utils.makeGUID(); + + collection.insert(newGUID, encryptPayload({ + id: newGUID, + bmkUri: "http://getfirefox.com/", + type: "bookmark", + title: "Get Firefox!", + parentName: "Folder 1", + parentid: newParentGUID, + tags: [], + }), Date.now() / 1000 + 10); + + // Now have the parent appear after (so when the record above is processed + // this is still unknown.) + collection.insert(newParentGUID, encryptPayload({ + id: newParentGUID, + type: "folder", + title: "Folder 1", + parentName: "A second folder", + parentid: folder2_guid, + children: [newGUID], + tags: [], + }), Date.now() / 1000 + 10); + // And also the update to "folder 2" that references the new parent. + collection.insert(folder2_guid, encryptPayload({ + id: folder2_guid, + type: "folder", + title: "A second folder", + parentName: "Bookmarks Toolbar", + parentid: "toolbar", + children: [newParentGUID], + tags: [], + }), Date.now() / 1000 + 10); + + _("Syncing so out-of-order records are processed."); + engine.lastSync = engine.lastSync - 0.01; + engine.sync(); + + // The intended parent did end up existing, so it should be parented + // correctly after de-duplication. + equal(getFolderChildrenIDs(folder1_id).length, 0); + let newParentID = store.idForGUID(newParentGUID); + let newID = store.idForGUID(newGUID); + deepEqual(getFolderChildrenIDs(newParentID), [newID]); + + // Make sure the validator thinks everything is hunky-dory. + yield validate(collection); + } finally { + yield cleanup(server); + } +}); + +add_task(function* test_dupe_reparented_to_future_arriving_parent_bookmark() { + _("Ensure that a bookmark we consider a dupe from a different parent that " + + "doesn't exist locally and doesn't appear in this Sync is handled correctly"); + + let { server, collection } = this.setup(); + + try { + // The parent folder and one bookmark in it. + let {id: folder1_id, guid: folder1_guid } = createFolder(bms.toolbarFolder, "Folder 1"); + let {id: bmk1_id, guid: bmk1_guid} = createBookmark(folder1_id, "http://getfirefox.com/", "Get Firefox!"); + // One more folder we'll use later. + let {id: folder2_id, guid: folder2_guid} = createFolder(bms.toolbarFolder, "A second folder"); + + do_print(`folder1=${folder1_guid}, bmk1=${bmk1_guid} folder2=${folder2_guid}`); + + engine.sync(); + + // We've added the bookmark, 2 folders plus "menu", "toolbar" and "unfiled" + equal(collection.count(), 6); + equal(getFolderChildrenIDs(folder1_id).length, 1); + + // Now create a new incoming record that looks alot like a dupe of the + // item in folder1_guid, but with a record that points to a parent with the + // same name, but a non-existing local ID. + let newGUID = Utils.makeGUID(); + let newParentGUID = Utils.makeGUID(); + + collection.insert(newGUID, encryptPayload({ + id: newGUID, + bmkUri: "http://getfirefox.com/", + type: "bookmark", + title: "Get Firefox!", + parentName: "Folder 1", + parentid: newParentGUID, + tags: [], + }), Date.now() / 1000 + 10); + + _("Syncing so new dupe record is processed"); + engine.lastSync = engine.lastSync - 0.01; + engine.sync(); + + // We should have logically deleted the dupe record. + equal(collection.count(), 7); + ok(getServerRecord(collection, bmk1_guid).deleted); + // and physically removed from the local store. + yield promiseNoLocalItem(bmk1_guid); + // The intended parent doesn't exist, so it remains in the original folder + equal(getFolderChildrenIDs(folder1_id).length, 1); + + // The record for folder1 on the server should reference the new GUID. + let serverRecord1 = getServerRecord(collection, folder1_guid); + ok(!serverRecord1.children.includes(bmk1_guid)); + ok(serverRecord1.children.includes(newGUID)); + + // As the incoming parent is missing the item should have been annotated + // with that missing parent. + equal(PlacesUtils.annotations.getItemAnnotation(store.idForGUID(newGUID), "sync/parent"), + newParentGUID); + + // Check the validator. Sadly, this is known to cause a mismatch between + // the server and client views of the tree. + let expected = [ + // We haven't fixed the incoming record that referenced the missing parent. + { name: "orphans", count: 1 }, + ]; + yield validate(collection, expected); + + // Now have the parent magically appear in a later sync - but + // it appears as being in a different parent from our existing "Folder 1", + // so the folder itself isn't duped. + collection.insert(newParentGUID, encryptPayload({ + id: newParentGUID, + type: "folder", + title: "Folder 1", + parentName: "A second folder", + parentid: folder2_guid, + children: [newGUID], + tags: [], + }), Date.now() / 1000 + 10); + // We also queue an update to "folder 2" that references the new parent. + collection.insert(folder2_guid, encryptPayload({ + id: folder2_guid, + type: "folder", + title: "A second folder", + parentName: "Bookmarks Toolbar", + parentid: "toolbar", + children: [newParentGUID], + tags: [], + }), Date.now() / 1000 + 10); + + _("Syncing so missing parent appears"); + engine.lastSync = engine.lastSync - 0.01; + engine.sync(); + + // The intended parent now does exist, so it should have been reparented. + equal(getFolderChildrenIDs(folder1_id).length, 0); + let newParentID = store.idForGUID(newParentGUID); + let newID = store.idForGUID(newGUID); + deepEqual(getFolderChildrenIDs(newParentID), [newID]); + + // validation now has different errors :( + expected = [ + // The validator reports multipleParents because: + // * The incoming record newParentGUID still (and correctly) references + // newGUID as a child. + // * Our original Folder1 was updated to include newGUID when it + // originally de-deuped and couldn't find the parent. + // * When the parent *did* eventually arrive we used the parent annotation + // to correctly reparent - but that reparenting process does not change + // the server record. + // Hence, newGUID is a child of both those server records :( + { name: "multipleParents", count: 1 }, + ]; + yield validate(collection, expected); + + } finally { + yield cleanup(server); + } +}); + +add_task(function* test_dupe_empty_folder() { + _("Ensure that an empty folder we consider a dupe is handled correctly."); + // Empty folders aren't particularly interesting in practice (as that seems + // an edge-case) but duping folders with items is broken - bug 1293163. + let { server, collection } = this.setup(); + + try { + // The folder we will end up duping away. + let {id: folder1_id, guid: folder1_guid } = createFolder(bms.toolbarFolder, "Folder 1"); + + engine.sync(); + + // We've added 1 folder, "menu", "toolbar" and "unfiled" + equal(collection.count(), 4); + + // Now create new incoming records that looks alot like a dupe of "Folder 1". + let newFolderGUID = Utils.makeGUID(); + collection.insert(newFolderGUID, encryptPayload({ + id: newFolderGUID, + type: "folder", + title: "Folder 1", + parentName: "Bookmarks Toolbar", + parentid: "toolbar", + children: [], + }), Date.now() / 1000 + 10); + + _("Syncing so new dupe records are processed"); + engine.lastSync = engine.lastSync - 0.01; + engine.sync(); + + yield validate(collection); + + // Collection now has one additional record - the logically deleted dupe. + equal(collection.count(), 5); + // original folder should be logically deleted. + ok(getServerRecord(collection, folder1_guid).deleted); + yield promiseNoLocalItem(folder1_guid); + } finally { + yield cleanup(server); + } +}); +// XXX - TODO - folders with children. Bug 1293163 diff --git a/services/sync/tests/unit/xpcshell.ini b/services/sync/tests/unit/xpcshell.ini index bec6d26cfa0a2..e7242feffb89b 100644 --- a/services/sync/tests/unit/xpcshell.ini +++ b/services/sync/tests/unit/xpcshell.ini @@ -144,6 +144,7 @@ tags = addons [test_addons_tracker.js] tags = addons [test_bookmark_batch_fail.js] +[test_bookmark_duping.js] [test_bookmark_engine.js] [test_bookmark_invalid.js] [test_bookmark_legacy_microsummaries_support.js] From 8a54c1a711ae0b2cce634fe3d7cc0007e5e205c1 Mon Sep 17 00:00:00 2001 From: Jean-Yves Avenard Date: Wed, 14 Sep 2016 16:45:52 +1000 Subject: [PATCH 03/29] Bug 1302632: P1. Set proper error code when readyState is HAVE_NOTHING. r=jwwang MozReview-Commit-ID: CMQkW5pPDF2 --HG-- extra : rebase_source : f0286af0b2daa2f91bbea66ab7bcf6de1a6d2207 --- dom/html/HTMLMediaElement.cpp | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/dom/html/HTMLMediaElement.cpp b/dom/html/HTMLMediaElement.cpp index 465fa4bdba6f2..f57431ee9f158 100644 --- a/dom/html/HTMLMediaElement.cpp +++ b/dom/html/HTMLMediaElement.cpp @@ -1042,9 +1042,6 @@ void HTMLMediaElement::AbortExistingLoads() void HTMLMediaElement::NoSupportedMediaSourceError() { - NS_ASSERTION(mNetworkState == NETWORK_LOADING, - "Not loading during source selection?"); - mError = new MediaError(this, MEDIA_ERR_SRC_NOT_SUPPORTED); ChangeNetworkState(nsIDOMHTMLMediaElement::NETWORK_NO_SOURCE); DispatchAsyncEvent(NS_LITERAL_STRING("error")); @@ -4416,7 +4413,11 @@ void HTMLMediaElement::NetworkError() if (mDecoder) { ShutdownDecoder(); } - Error(MEDIA_ERR_NETWORK); + if (mReadyState == nsIDOMHTMLMediaElement::HAVE_NOTHING) { + NoSupportedMediaSourceError(); + } else { + Error(MEDIA_ERR_NETWORK); + } } void HTMLMediaElement::DecodeError(const MediaResult& aError) @@ -4442,6 +4443,8 @@ void HTMLMediaElement::DecodeError(const MediaResult& aError) } else { NS_WARNING("Should know the source we were loading from!"); } + } else if (mReadyState == nsIDOMHTMLMediaElement::HAVE_NOTHING) { + NoSupportedMediaSourceError(); } else { Error(MEDIA_ERR_DECODE, aError); } From 9b10344e7c6fb2e54c47c4bd66bc88c94193070d Mon Sep 17 00:00:00 2001 From: Jean-Yves Avenard Date: Wed, 14 Sep 2016 16:49:56 +1000 Subject: [PATCH 04/29] Bug 1302632: P2. Only have Description return a non-empty string in case of error. r=jwwang Will simplify the code in the following patch, not having to perform unnecessary test. MozReview-Commit-ID: 5zUzSZgzrG9 --HG-- extra : rebase_source : c409a70d1aa5975b1a990a884856b3b5712bc190 --- dom/media/MediaResult.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/dom/media/MediaResult.h b/dom/media/MediaResult.h index 593f0a8c553cd..214ad7f5a933c 100644 --- a/dom/media/MediaResult.h +++ b/dom/media/MediaResult.h @@ -48,6 +48,9 @@ class MediaResult nsCString Description() const { + if (NS_SUCCEEDED(mCode)) { + return nsCString(); + } return nsPrintfCString("0x%08x: %s", mCode, mMessage.get()); } From 895efe36db263ad313e09b522a51ec0318e7b8cc Mon Sep 17 00:00:00 2001 From: Jean-Yves Avenard Date: Thu, 15 Sep 2016 16:17:10 +1000 Subject: [PATCH 05/29] Bug 1302632: P3. Provide failures details to error attribute. r=jwwang MozReview-Commit-ID: 3eIXOF96UR4 --HG-- extra : rebase_source : eeeb298d46bd8c6957560e475b7b1c4728a392dc --- dom/html/HTMLMediaElement.cpp | 16 ++++++---------- dom/html/HTMLMediaElement.h | 4 ++-- 2 files changed, 8 insertions(+), 12 deletions(-) diff --git a/dom/html/HTMLMediaElement.cpp b/dom/html/HTMLMediaElement.cpp index f57431ee9f158..84dab0293c0f8 100644 --- a/dom/html/HTMLMediaElement.cpp +++ b/dom/html/HTMLMediaElement.cpp @@ -1040,9 +1040,9 @@ void HTMLMediaElement::AbortExistingLoads() mPendingEvents.Clear(); } -void HTMLMediaElement::NoSupportedMediaSourceError() +void HTMLMediaElement::NoSupportedMediaSourceError(const nsACString& aErrorDetails) { - mError = new MediaError(this, MEDIA_ERR_SRC_NOT_SUPPORTED); + mError = new MediaError(this, MEDIA_ERR_SRC_NOT_SUPPORTED, aErrorDetails); ChangeNetworkState(nsIDOMHTMLMediaElement::NETWORK_NO_SOURCE); DispatchAsyncEvent(NS_LITERAL_STRING("error")); ChangeDelayLoadStatus(false); @@ -4444,9 +4444,9 @@ void HTMLMediaElement::DecodeError(const MediaResult& aError) NS_WARNING("Should know the source we were loading from!"); } } else if (mReadyState == nsIDOMHTMLMediaElement::HAVE_NOTHING) { - NoSupportedMediaSourceError(); + NoSupportedMediaSourceError(aError.Description()); } else { - Error(MEDIA_ERR_DECODE, aError); + Error(MEDIA_ERR_DECODE, aError.Description()); } } @@ -4461,7 +4461,7 @@ void HTMLMediaElement::LoadAborted() } void HTMLMediaElement::Error(uint16_t aErrorCode, - const MediaResult& aErrorDetails) + const nsACString& aErrorDetails) { NS_ASSERTION(aErrorCode == MEDIA_ERR_DECODE || aErrorCode == MEDIA_ERR_NETWORK || @@ -4474,11 +4474,7 @@ void HTMLMediaElement::Error(uint16_t aErrorCode, if (mError) { return; } - nsCString message; - if (NS_FAILED(aErrorDetails)) { - message = aErrorDetails.Description(); - } - mError = new MediaError(this, aErrorCode, message); + mError = new MediaError(this, aErrorCode, aErrorDetails); DispatchAsyncEvent(NS_LITERAL_STRING("error")); if (mReadyState == nsIDOMHTMLMediaElement::HAVE_NOTHING) { diff --git a/dom/html/HTMLMediaElement.h b/dom/html/HTMLMediaElement.h index cd673782e081c..0c7ad442f4f1a 100644 --- a/dom/html/HTMLMediaElement.h +++ b/dom/html/HTMLMediaElement.h @@ -961,7 +961,7 @@ class HTMLMediaElement : public nsGenericHTMLElement, * state to NETWORK_NO_SOURCE, and sends error event with code * MEDIA_ERR_SRC_NOT_SUPPORTED. */ - void NoSupportedMediaSourceError(); + void NoSupportedMediaSourceError(const nsACString& aErrorDetails = nsCString()); /** * Attempts to load resources from the children. This is a @@ -1127,7 +1127,7 @@ class HTMLMediaElement : public nsGenericHTMLElement, * Resets the media element for an error condition as per aErrorCode. * aErrorCode must be one of nsIDOMHTMLMediaError codes. */ - void Error(uint16_t aErrorCode, const MediaResult& aErrorDetails = NS_OK); + void Error(uint16_t aErrorCode, const nsACString& aErrorDetails = nsCString()); /** * Returns the URL spec of the currentSrc. From 26629fd77e310ca3b3d40e0a470d6d8b397e3438 Mon Sep 17 00:00:00 2001 From: Jean-Yves Avenard Date: Thu, 15 Sep 2016 16:20:43 +1000 Subject: [PATCH 06/29] Bug 1302632: P4. Do not change network state to NETWORK_EMPTY. r=jwwang There's no such step defined in the spec. Despite, this code can no longer be called when readyState is HAVE_NOTHING MozReview-Commit-ID: 2fDoNHt1COp --HG-- extra : rebase_source : d8b79acee85b00331f55153626038e26cd4a72c7 --- dom/html/HTMLMediaElement.cpp | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/dom/html/HTMLMediaElement.cpp b/dom/html/HTMLMediaElement.cpp index 84dab0293c0f8..8f32d8f43d4e9 100644 --- a/dom/html/HTMLMediaElement.cpp +++ b/dom/html/HTMLMediaElement.cpp @@ -4467,6 +4467,8 @@ void HTMLMediaElement::Error(uint16_t aErrorCode, aErrorCode == MEDIA_ERR_NETWORK || aErrorCode == MEDIA_ERR_ABORTED, "Only use MediaError codes!"); + NS_ASSERTION(mReadyState > HAVE_NOTHING, + "Shouldn't be called when readyState is HAVE_NOTHING"); // Since we have multiple paths calling into DecodeError, e.g. // MediaKeys::Terminated and EMEH264Decoder::Error. We should take the 1st @@ -4477,12 +4479,7 @@ void HTMLMediaElement::Error(uint16_t aErrorCode, mError = new MediaError(this, aErrorCode, aErrorDetails); DispatchAsyncEvent(NS_LITERAL_STRING("error")); - if (mReadyState == nsIDOMHTMLMediaElement::HAVE_NOTHING) { - ChangeNetworkState(nsIDOMHTMLMediaElement::NETWORK_EMPTY); - DispatchAsyncEvent(NS_LITERAL_STRING("emptied")); - } else { - ChangeNetworkState(nsIDOMHTMLMediaElement::NETWORK_IDLE); - } + ChangeNetworkState(nsIDOMHTMLMediaElement::NETWORK_IDLE); ChangeDelayLoadStatus(false); UpdateAudioChannelPlayingState(); } From 4387906eaefee9802d2271265c4f9ef81578a99a Mon Sep 17 00:00:00 2001 From: Jean-Yves Avenard Date: Fri, 16 Sep 2016 11:14:07 +1000 Subject: [PATCH 07/29] Bug 1302632: P5. Update mochitests. r=gerald The tests expected that the error code would be MEDIA_ERR_DECODE whenever we fail to open a video. However, MEDIA_ERR_DECODE is to be used only when "An error of some description occurred while decoding the media resource, after the resource was established to be usable." All those files have errors in their metadata. Which makes the resource unusable to start with. Similarly, networkState would be set to NETWORK_NO_SOURCE if the metadata couldn't be read. MozReview-Commit-ID: KXVJmD3ZQlT --HG-- extra : rebase_source : 1ec3d7e764d832702e662f0b650363498e0b0761 --- dom/media/test/test_decode_error.html | 6 +++++- dom/media/test/test_error_in_video_document.html | 8 +++----- dom/media/test/test_invalid_reject.html | 9 +++++++-- 3 files changed, 15 insertions(+), 8 deletions(-) diff --git a/dom/media/test/test_decode_error.html b/dom/media/test/test_decode_error.html index 2a5d9998c3b58..bb48d4343670c 100644 --- a/dom/media/test/test_decode_error.html +++ b/dom/media/test/test_decode_error.html @@ -26,7 +26,11 @@ is(event.type, "error", "Expected event of type 'error'"); ok(el.error, "Element 'error' attr expected to have a value"); ok(el.error instanceof MediaError, "Element 'error' attr expected to be MediaError"); - is(el.error.code, MediaError.MEDIA_ERR_DECODE, "Expected a decode error"); + if (v.readyState == v.HAVE_NOTHING) { + is(el.error.code, MediaError.MEDIA_ERR_SRC_NOT_SUPPORTED, "Expected media not supported error"); + } else { + is(el.error.code, MediaError.MEDIA_ERR_DECODE, "Expected a decode error"); + } ok(typeof el.error.message === 'string' || el.error.essage instanceof String, "Element 'message' attr expected to be a string"); ok(el.error.message.length > 0, "Element 'message' attr has content"); el._sawError = true; diff --git a/dom/media/test/test_error_in_video_document.html b/dom/media/test/test_error_in_video_document.html index ed434356c35b1..f0404c4d81ddf 100644 --- a/dom/media/test/test_error_in_video_document.html +++ b/dom/media/test/test_error_in_video_document.html @@ -31,13 +31,11 @@ // Debug info for Bug 608634 ok(true, "iframe src=" + document.body.getElementsByTagName("iframe")[0].src); - is(v.readyState, 0, "Ready state"); + is(v.readyState, v.HAVE_NOTHING, "Ready state"); - is(v.networkState, 0, "Network state"); isnot(v.error, null, "Error object"); - if (v.error) - is(v.error.code, MediaError.MEDIA_ERR_DECODE, "Error code"); - + is(v.networkState, v.NETWORK_NO_SOURCE, "Network state"); + is(v.error.code, MediaError.MEDIA_ERR_SRC_NOT_SUPPORTED, "Expected media not supported error"); SimpleTest.finish(); } diff --git a/dom/media/test/test_invalid_reject.html b/dom/media/test/test_invalid_reject.html index 20a2eef9cdcb0..ac9b102de2640 100644 --- a/dom/media/test/test_invalid_reject.html +++ b/dom/media/test/test_invalid_reject.html @@ -32,8 +32,13 @@ // Seeing a decoder error is a success. v.addEventListener("error", function onerror(e) { - is(v.error.code, v.error.MEDIA_ERR_DECODE, - "decoder should reject " + test.name); + if (v.readyState == v.HAVE_NOTHING) { + is(v.error.code, v.error.MEDIA_ERR_SRC_NOT_SUPPORTED, + "decoder should reject " + test.name); + } else { + is(v.error.code, v.error.MEDIA_ERR_DECODE, + "decoder should reject " + test.name); + } v.removeEventListener('error', onerror, false); manager.finished(token); }); From 2f20475d5e2e311fb5d652cb3a1302fcf396f538 Mon Sep 17 00:00:00 2001 From: Henrik Skupin Date: Tue, 13 Sep 2016 17:16:51 +0200 Subject: [PATCH 08/29] Bug 1302364 - Update TEST_MANIFESTS documentation for install_root in resolve_tests(). r=gps MozReview-Commit-ID: 9EXT3Jakwx1 --HG-- extra : rebase_source : ba1f6afcaa35103a52e5bbad7618fa2c9f08f2a1 --- python/mozbuild/mozbuild/testing.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/python/mozbuild/mozbuild/testing.py b/python/mozbuild/mozbuild/testing.py index 19e3fa4fa80a2..3c95f90f8a232 100644 --- a/python/mozbuild/mozbuild/testing.py +++ b/python/mozbuild/mozbuild/testing.py @@ -260,10 +260,12 @@ def resolve_tests(self, cwd=None, **kwargs): # Keys are variable prefixes and values are tuples describing how these # manifests should be handled: # -# (flavor, install_prefix, package_tests) +# (flavor, install_root, install_subdir, package_tests) # # flavor identifies the flavor of this test. -# install_prefix is the path prefix of where to install the files in +# install_root is the path prefix to install the files starting from the root +# directory and not as specified by the manifest location. (bug 972168) +# install_subdir is the path of where to install the files in # the tests directory. # package_tests indicates whether to package test files into the test # package; suites that compile the test files should not install From 3418dc5d58a243e3508a67fe3015bc315357105a Mon Sep 17 00:00:00 2001 From: Henrik Skupin Date: Tue, 13 Sep 2016 17:21:52 +0200 Subject: [PATCH 09/29] Bug 1302364 - Include firefox-ui and puppeteer in all-tests.json, and allow to run tests via "mach test". r=gps MozReview-Commit-ID: EwONsQSgAym --HG-- extra : rebase_source : 61870a85c3f749ac3da202e08c2ba91fa87a261b --- python/mozbuild/mozbuild/frontend/context.py | 12 ++++++++++++ python/mozbuild/mozbuild/testing.py | 3 +++ testing/firefox-ui/mach_commands.py | 10 ++++++++-- testing/firefox-ui/moz.build | 11 +++++++++++ testing/mach_commands.py | 18 ++++++++++++++++++ toolkit/toolkit.mozbuild | 5 ++++- 6 files changed, 56 insertions(+), 3 deletions(-) create mode 100644 testing/firefox-ui/moz.build diff --git a/python/mozbuild/mozbuild/frontend/context.py b/python/mozbuild/mozbuild/frontend/context.py index ff1b8f980bc78..9eb847faee067 100644 --- a/python/mozbuild/mozbuild/frontend/context.py +++ b/python/mozbuild/mozbuild/frontend/context.py @@ -1515,6 +1515,18 @@ def aggregate(files): """List of manifest files defining Android instrumentation tests. """), + 'FIREFOX_UI_FUNCTIONAL_MANIFESTS': (ManifestparserManifestList, list, + """List of manifest files defining firefox-ui-functional tests. + """), + + 'FIREFOX_UI_UPDATE_MANIFESTS': (ManifestparserManifestList, list, + """List of manifest files defining firefox-ui-update tests. + """), + + 'PUPPETEER_FIREFOX_MANIFESTS': (ManifestparserManifestList, list, + """List of manifest files defining puppeteer unit tests for Firefox. + """), + 'MARIONETTE_LAYOUT_MANIFESTS': (ManifestparserManifestList, list, """List of manifest files defining marionette-layout tests. """), diff --git a/python/mozbuild/mozbuild/testing.py b/python/mozbuild/mozbuild/testing.py index 3c95f90f8a232..d3722fac655fb 100644 --- a/python/mozbuild/mozbuild/testing.py +++ b/python/mozbuild/mozbuild/testing.py @@ -277,6 +277,9 @@ def resolve_tests(self, cwd=None, **kwargs): ANDROID_INSTRUMENTATION=('instrumentation', 'instrumentation', '.', False), JETPACK_PACKAGE=('jetpack-package', 'testing/mochitest', 'jetpack-package', True), JETPACK_ADDON=('jetpack-addon', 'testing/mochitest', 'jetpack-addon', False), + FIREFOX_UI_FUNCTIONAL=('firefox-ui-functional', 'firefox-ui', '.', False), + FIREFOX_UI_UPDATE=('firefox-ui-update', 'firefox-ui', '.', False), + PUPPETEER_FIREFOX=('firefox-ui-functional', 'firefox-ui', '.', False), # marionette tests are run from the srcdir # TODO(ato): make packaging work as for other test suites diff --git a/testing/firefox-ui/mach_commands.py b/testing/firefox-ui/mach_commands.py index 9d5281d46917e..368b673a22c91 100644 --- a/testing/firefox-ui/mach_commands.py +++ b/testing/firefox-ui/mach_commands.py @@ -66,8 +66,14 @@ def run_firefox_ui_test(testtype=None, topsrcdir=None, **kwargs): if not kwargs['server_root']: kwargs['server_root'] = os.path.join(fxui_dir, 'resources') - # If no tests have been selected, set default ones - if not kwargs.get('tests'): + # If called via "mach test" a dictionary of tests is passed in + if 'test_objects' in kwargs: + tests = [] + for obj in kwargs['test_objects']: + tests.append(obj['file_relpath']) + kwargs['tests'] = tests + elif not kwargs.get('tests'): + # If no tests have been selected, set default ones kwargs['tests'] = [os.path.join(fxui_dir, 'tests', test) for test in test_types[testtype]['default_tests']] diff --git a/testing/firefox-ui/moz.build b/testing/firefox-ui/moz.build new file mode 100644 index 0000000000000..0f4a3a2e7beb0 --- /dev/null +++ b/testing/firefox-ui/moz.build @@ -0,0 +1,11 @@ +# This Source Code Form is subject to the terms of the Mozilla Public +# License, v. 2.0. If a copy of the MPL was not distributed with this +# file, You can obtain one at http://mozilla.org/MPL/2.0/. + +FIREFOX_UI_FUNCTIONAL_MANIFESTS += ["tests/functional/manifest.ini"] +FIREFOX_UI_UPDATE_MANIFESTS += ["tests/update/manifest.ini"] +# Bug 1272145: Move to testing/puppeteer/firefox +PUPPETEER_FIREFOX_MANIFESTS += ["tests/puppeteer/manifest.ini"] + +with Files("**"): + BUG_COMPONENT = ("Testing", "Firefox UI Tests") diff --git a/testing/mach_commands.py b/testing/mach_commands.py index 4eb19a6618a45..1b21300262711 100644 --- a/testing/mach_commands.py +++ b/testing/mach_commands.py @@ -61,6 +61,16 @@ 'mach_command': 'crashtest-ipc', 'kwargs': {'test_file': None}, }, + 'firefox-ui-functional': { + 'aliases': ('Fxfn',), + 'mach_command': 'firefox-ui-functional', + 'kwargs': {}, + }, + 'firefox-ui-update': { + 'aliases': ('Fxup',), + 'mach_command': 'firefox-ui-update', + 'kwargs': {}, + }, 'jetpack': { 'aliases': ('J',), 'mach_command': 'jetpack-test', @@ -143,6 +153,14 @@ 'mach_command': 'mochitest', 'kwargs': {'flavor': 'chrome', 'test_paths': []}, }, + 'firefox-ui-functional': { + 'mach_command': 'firefox-ui-functional', + 'kwargs': {'tests': []}, + }, + 'firefox-ui-update': { + 'mach_command': 'firefox-ui-update', + 'kwargs': {'tests': []}, + }, 'marionette': { 'mach_command': 'marionette-test', 'kwargs': {'tests': []}, diff --git a/toolkit/toolkit.mozbuild b/toolkit/toolkit.mozbuild index eaae9d3d781bb..05c42fa44ba31 100644 --- a/toolkit/toolkit.mozbuild +++ b/toolkit/toolkit.mozbuild @@ -160,7 +160,10 @@ if 'gtk' in CONFIG['MOZ_WIDGET_TOOLKIT']: DIRS += ['/addon-sdk'] if CONFIG['ENABLE_MARIONETTE'] or CONFIG['MOZ_WIDGET_TOOLKIT'] not in ('gonk', 'android'): - DIRS += ['/testing/marionette'] + DIRS += [ + '/testing/firefox-ui', + '/testing/marionette', + ] DIRS += [ '/tools/quitter', From 5be6837a7d99655a9c332f8798755761f2c8c9a8 Mon Sep 17 00:00:00 2001 From: Gerald Squelart Date: Thu, 15 Sep 2016 00:15:59 -0700 Subject: [PATCH 10/29] Bug 1303255 - Use correct dec-doc messages on Vista - r=Gijs MozReview-Commit-ID: FeaFlvaL8t4 --HG-- extra : rebase_source : 36c92a53acbfc654b82692e2a250002f4389c6a4 --- browser/base/content/browser-media.js | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/browser/base/content/browser-media.js b/browser/base/content/browser-media.js index 2e62f936e94bb..36b2c019a5ec5 100644 --- a/browser/base/content/browser-media.js +++ b/browser/base/content/browser-media.js @@ -219,12 +219,18 @@ let gDecoderDoctorHandler = { if (AppConstants.isPlatformAndVersionAtMost("win", "5.9")) { return gNavigatorBundle.getString("decoder.noCodecsXP.message"); } + if (!AppConstants.isPlatformAndVersionAtLeast("win", "6.1")) { + return gNavigatorBundle.getString("decoder.noCodecsVista.message"); + } return gNavigatorBundle.getString("decoder.noCodecs.message"); } if (type == "platform-decoder-not-found") { - if (AppConstants.isPlatformAndVersionAtLeast("win", "6")) { + if (AppConstants.isPlatformAndVersionAtLeast("win", "6.1")) { return gNavigatorBundle.getString("decoder.noHWAcceleration.message"); } + if (AppConstants.isPlatformAndVersionAtLeast("win", "6")) { + return gNavigatorBundle.getString("decoder.noHWAccelerationVista.message"); + } if (AppConstants.platform == "linux") { return gNavigatorBundle.getString("decoder.noCodecsLinux.message"); } From ae4301ef96a39090836f377cafc93f23a15698a6 Mon Sep 17 00:00:00 2001 From: Gijs Kruitbosch Date: Mon, 18 Jul 2016 16:46:45 +0100 Subject: [PATCH 11/29] Bug 1285041 - ignore locking when trying to read chrome DB file, r=mak MozReview-Commit-ID: 89f0YCxxgC8 --HG-- extra : rebase_source : 53efa0f0258728df54fc17a773cbf74c712b51e4 --- .../migration/ChromeProfileMigrator.js | 52 +++++++++++++-- storage/mozStorageConnection.cpp | 16 ++++- storage/mozStorageConnection.h | 15 ++++- storage/mozStorageService.cpp | 65 +++++++++++++------ storage/test/unit/test_storage_connection.js | 19 +++++- toolkit/modules/Sqlite.jsm | 24 ++++++- 6 files changed, 156 insertions(+), 35 deletions(-) diff --git a/browser/components/migration/ChromeProfileMigrator.js b/browser/components/migration/ChromeProfileMigrator.js index aa8fea0dc1ad0..71d0b0f667c97 100644 --- a/browser/components/migration/ChromeProfileMigrator.js +++ b/browser/components/migration/ChromeProfileMigrator.js @@ -311,19 +311,59 @@ function GetHistoryResource(aProfileFolder) { if (!historyFile.exists()) return null; + function getRows(dbOptions) { + const RETRYLIMIT = 10; + const RETRYINTERVAL = 100; + return Task.spawn(function* innerGetRows() { + let rows = null; + for (let retryCount = RETRYLIMIT; retryCount && !rows; retryCount--) { + // Attempt to get the rows. If this succeeds, we will bail out of the loop, + // close the database in a failsafe way, and pass the rows back. + // If fetching the rows throws, we will wait RETRYINTERVAL ms + // and try again. This will repeat a maximum of RETRYLIMIT times. + let db; + let didOpen = false; + let exceptionSeen; + try { + db = yield Sqlite.openConnection(dbOptions); + didOpen = true; + rows = yield db.execute(`SELECT url, title, last_visit_time, typed_count + FROM urls WHERE hidden = 0`); + } catch (ex) { + if (!exceptionSeen) { + Cu.reportError(ex); + } + exceptionSeen = ex; + } finally { + try { + if (didOpen) { + yield db.close(); + } + } catch (ex) {} + } + if (exceptionSeen) { + yield new Promise(resolve => setTimeout(resolve, RETRYINTERVAL)); + } + } + if (!rows) { + throw new Error("Couldn't get rows from the Chrome history database."); + } + return rows; + }); + } + return { type: MigrationUtils.resourceTypes.HISTORY, migrate(aCallback) { Task.spawn(function* () { - let db = yield Sqlite.openConnection({ + let dbOptions = { + readOnly: true, + ignoreLockingMode: true, path: historyFile.path - }); - - let rows = yield db.execute(`SELECT url, title, last_visit_time, typed_count - FROM urls WHERE hidden = 0`); - yield db.close(); + }; + let rows = yield getRows(dbOptions); let places = []; for (let row of rows) { try { diff --git a/storage/mozStorageConnection.cpp b/storage/mozStorageConnection.cpp index 00e0f0bb81479..0da908a429bd2 100644 --- a/storage/mozStorageConnection.cpp +++ b/storage/mozStorageConnection.cpp @@ -472,7 +472,8 @@ class AsyncInitializeClone final: public Runnable Connection::Connection(Service *aService, int aFlags, - bool aAsyncOnly) + bool aAsyncOnly, + bool aIgnoreLockingMode) : sharedAsyncExecutionMutex("Connection::sharedAsyncExecutionMutex") , sharedDBMutex("Connection::sharedDBMutex") , threadOpenedOn(do_GetCurrentThread()) @@ -485,9 +486,12 @@ Connection::Connection(Service *aService, , mTransactionInProgress(false) , mProgressHandler(nullptr) , mFlags(aFlags) +, mIgnoreLockingMode(aIgnoreLockingMode) , mStorageService(aService) , mAsyncOnly(aAsyncOnly) { + MOZ_ASSERT(!mIgnoreLockingMode || mFlags & SQLITE_OPEN_READONLY, + "Can't ignore locking for a non-readonly connection!"); mStorageService->registerConnection(this); } @@ -577,6 +581,7 @@ nsresult Connection::initialize() { NS_ASSERTION (!mDBConn, "Initialize called on already opened database!"); + MOZ_ASSERT(!mIgnoreLockingMode, "Can't ignore locking on an in-memory db."); PROFILER_LABEL("mozStorageConnection", "initialize", js::ProfileEntry::Category::STORAGE); @@ -610,8 +615,15 @@ Connection::initialize(nsIFile *aDatabaseFile) nsresult rv = aDatabaseFile->GetPath(path); NS_ENSURE_SUCCESS(rv, rv); +#ifdef XP_WIN + static const char* sIgnoreLockingVFS = "win32-none"; +#else + static const char* sIgnoreLockingVFS = "unix-none"; +#endif + const char* vfs = mIgnoreLockingMode ? sIgnoreLockingVFS : nullptr; + int srv = ::sqlite3_open_v2(NS_ConvertUTF16toUTF8(path).get(), &mDBConn, - mFlags, nullptr); + mFlags, vfs); if (srv != SQLITE_OK) { mDBConn = nullptr; return convertResultCode(srv); diff --git a/storage/mozStorageConnection.h b/storage/mozStorageConnection.h index bb1b6dd4cf8e6..979ac6436e8de 100644 --- a/storage/mozStorageConnection.h +++ b/storage/mozStorageConnection.h @@ -69,8 +69,16 @@ class Connection final : public mozIStorageConnection * - |mozIStorageAsyncConnection|; * If |false|, the result also implements synchronous interface: * - |mozIStorageConnection|. + * @param aIgnoreLockingMode + * If |true|, ignore locks in force on the file. Only usable with + * read-only connections. Defaults to false. + * Use with extreme caution. If sqlite ignores locks, reads may fail + * indicating database corruption (the database won't actually be + * corrupt) or produce wrong results without any indication that has + * happened. */ - Connection(Service *aService, int aFlags, bool aAsyncOnly); + Connection(Service *aService, int aFlags, bool aAsyncOnly, + bool aIgnoreLockingMode = false); /** * Creates the connection to an in-memory database. @@ -356,6 +364,11 @@ class Connection final : public mozIStorageConnection */ const int mFlags; + /** + * Stores whether we should ask sqlite3_open_v2 to ignore locking. + */ + const bool mIgnoreLockingMode; + // This is here for two reasons: 1) It's used to make sure that the // connections do not outlive the service. 2) Our custom collating functions // call its localeCompareStrings() method. diff --git a/storage/mozStorageService.cpp b/storage/mozStorageService.cpp index 1d8f9905b9244..5536d5b254ddf 100644 --- a/storage/mozStorageService.cpp +++ b/storage/mozStorageService.cpp @@ -748,11 +748,43 @@ Service::OpenAsyncDatabase(nsIVariant *aDatabaseStore, NS_ENSURE_ARG(aDatabaseStore); NS_ENSURE_ARG(aCallback); - nsCOMPtr storageFile; - int flags = SQLITE_OPEN_READWRITE; + nsresult rv; + bool shared = false; + bool readOnly = false; + bool ignoreLockingMode = false; + int32_t growthIncrement = -1; + +#define FAIL_IF_SET_BUT_INVALID(rv)\ + if (NS_FAILED(rv) && rv != NS_ERROR_NOT_AVAILABLE) { \ + return NS_ERROR_INVALID_ARG; \ + } + + // Deal with options first: + if (aOptions) { + rv = aOptions->GetPropertyAsBool(NS_LITERAL_STRING("readOnly"), &readOnly); + FAIL_IF_SET_BUT_INVALID(rv); + + rv = aOptions->GetPropertyAsBool(NS_LITERAL_STRING("ignoreLockingMode"), + &ignoreLockingMode); + FAIL_IF_SET_BUT_INVALID(rv); + // Specifying ignoreLockingMode will force use of the readOnly flag: + if (ignoreLockingMode) { + readOnly = true; + } + + rv = aOptions->GetPropertyAsBool(NS_LITERAL_STRING("shared"), &shared); + FAIL_IF_SET_BUT_INVALID(rv); + + // NB: we re-set to -1 if we don't have a storage file later on. + rv = aOptions->GetPropertyAsInt32(NS_LITERAL_STRING("growthIncrement"), + &growthIncrement); + FAIL_IF_SET_BUT_INVALID(rv); + } + int flags = readOnly ? SQLITE_OPEN_READONLY : SQLITE_OPEN_READWRITE; + nsCOMPtr storageFile; nsCOMPtr dbStore; - nsresult rv = aDatabaseStore->GetAsISupports(getter_AddRefs(dbStore)); + rv = aDatabaseStore->GetAsISupports(getter_AddRefs(dbStore)); if (NS_SUCCEEDED(rv)) { // Generally, aDatabaseStore holds the database nsIFile. storageFile = do_QueryInterface(dbStore, &rv); @@ -763,17 +795,12 @@ Service::OpenAsyncDatabase(nsIVariant *aDatabaseStore, rv = storageFile->Clone(getter_AddRefs(storageFile)); MOZ_ASSERT(NS_SUCCEEDED(rv)); - // Ensure that SQLITE_OPEN_CREATE is passed in for compatibility reasons. - flags |= SQLITE_OPEN_CREATE; - - // Extract and apply the shared-cache option. - bool shared = false; - if (aOptions) { - rv = aOptions->GetPropertyAsBool(NS_LITERAL_STRING("shared"), &shared); - if (NS_FAILED(rv) && rv != NS_ERROR_NOT_AVAILABLE) { - return NS_ERROR_INVALID_ARG; - } + if (!readOnly) { + // Ensure that SQLITE_OPEN_CREATE is passed in for compatibility reasons. + flags |= SQLITE_OPEN_CREATE; } + + // Apply the shared-cache option. flags |= shared ? SQLITE_OPEN_SHAREDCACHE : SQLITE_OPEN_PRIVATECACHE; } else { // Sometimes, however, it's a special database name. @@ -787,17 +814,13 @@ Service::OpenAsyncDatabase(nsIVariant *aDatabaseStore, // connection to use a memory DB. } - int32_t growthIncrement = -1; - if (aOptions && storageFile) { - rv = aOptions->GetPropertyAsInt32(NS_LITERAL_STRING("growthIncrement"), - &growthIncrement); - if (NS_FAILED(rv) && rv != NS_ERROR_NOT_AVAILABLE) { - return NS_ERROR_INVALID_ARG; - } + if (!storageFile && growthIncrement >= 0) { + return NS_ERROR_INVALID_ARG; } // Create connection on this thread, but initialize it on its helper thread. - RefPtr msc = new Connection(this, flags, true); + RefPtr msc = new Connection(this, flags, true, + ignoreLockingMode); nsCOMPtr target = msc->getAsyncExecutionTarget(); MOZ_ASSERT(target, "Cannot initialize a connection that has been closed already"); diff --git a/storage/test/unit/test_storage_connection.js b/storage/test/unit/test_storage_connection.js index 2d6341f257738..bacd721c9ce59 100644 --- a/storage/test/unit/test_storage_connection.js +++ b/storage/test/unit/test_storage_connection.js @@ -355,12 +355,27 @@ add_task(function* test_open_async() { yield standardAsyncTest(openAsyncDatabase("memory"), "in-memory database", true); yield standardAsyncTest(openAsyncDatabase("memory", - {shared: false, growthIncrement: 54}), + {shared: false}), "in-memory database and options", true); - do_print("Testing async opening with bogus options 1"); + do_print("Testing async opening with bogus options 0"); let raised = false; let adb = null; + + try { + adb = yield openAsyncDatabase("memory", {shared: false, growthIncrement: 54}); + } catch (ex) { + raised = true; + } finally { + if (adb) { + yield asyncClose(adb); + } + } + do_check_true(raised); + + do_print("Testing async opening with bogus options 1"); + raised = false; + adb = null; try { adb = yield openAsyncDatabase(getTestDB(), {shared: "forty-two"}); } catch (ex) { diff --git a/toolkit/modules/Sqlite.jsm b/toolkit/modules/Sqlite.jsm index f024ec8bd1959..e8d986c0e5013 100644 --- a/toolkit/modules/Sqlite.jsm +++ b/toolkit/modules/Sqlite.jsm @@ -868,6 +868,15 @@ ConnectionData.prototype = Object.freeze({ * *not* a timer on the idle service and this could fire while the * application is active. * + * readOnly -- (bool) Whether to open the database with SQLITE_OPEN_READONLY + * set. If used, writing to the database will fail. Defaults to false. + * + * ignoreLockingMode -- (bool) Whether to ignore locks on the database held + * by other connections. If used, implies readOnly. Defaults to false. + * USE WITH EXTREME CAUTION. This mode WILL produce incorrect results or + * return "false positive" corruption errors if other connections write + * to the DB at the same time. + * * FUTURE options to control: * * special named databases @@ -915,12 +924,21 @@ function openConnection(options) { log.info("Opening database: " + path + " (" + identifier + ")"); return new Promise((resolve, reject) => { - let dbOptions = null; + let dbOptions = Cc["@mozilla.org/hash-property-bag;1"]. + createInstance(Ci.nsIWritablePropertyBag); if (!sharedMemoryCache) { - dbOptions = Cc["@mozilla.org/hash-property-bag;1"]. - createInstance(Ci.nsIWritablePropertyBag); dbOptions.setProperty("shared", false); } + if (options.readOnly) { + dbOptions.setProperty("readOnly", true); + } + if (options.ignoreLockingMode) { + dbOptions.setProperty("ignoreLockingMode", true); + dbOptions.setProperty("readOnly", true); + } + + dbOptions = dbOptions.enumerator.hasMoreElements() ? dbOptions : null; + Services.storage.openAsyncDatabase(file, dbOptions, (status, connection) => { if (!connection) { log.warn(`Could not open connection to ${path}: ${status}`); From 99e7648673df677cb1af23c9f1ea5ed546071c25 Mon Sep 17 00:00:00 2001 From: Sebastian Hengst Date: Fri, 16 Sep 2016 11:24:29 +0200 Subject: [PATCH 12/29] Backed out changeset 1df8bde64853 (bug 1302364) --- python/mozbuild/mozbuild/frontend/context.py | 12 ------------ python/mozbuild/mozbuild/testing.py | 3 --- testing/firefox-ui/mach_commands.py | 10 ++-------- testing/firefox-ui/moz.build | 11 ----------- testing/mach_commands.py | 18 ------------------ toolkit/toolkit.mozbuild | 5 +---- 6 files changed, 3 insertions(+), 56 deletions(-) delete mode 100644 testing/firefox-ui/moz.build diff --git a/python/mozbuild/mozbuild/frontend/context.py b/python/mozbuild/mozbuild/frontend/context.py index 9eb847faee067..ff1b8f980bc78 100644 --- a/python/mozbuild/mozbuild/frontend/context.py +++ b/python/mozbuild/mozbuild/frontend/context.py @@ -1515,18 +1515,6 @@ def aggregate(files): """List of manifest files defining Android instrumentation tests. """), - 'FIREFOX_UI_FUNCTIONAL_MANIFESTS': (ManifestparserManifestList, list, - """List of manifest files defining firefox-ui-functional tests. - """), - - 'FIREFOX_UI_UPDATE_MANIFESTS': (ManifestparserManifestList, list, - """List of manifest files defining firefox-ui-update tests. - """), - - 'PUPPETEER_FIREFOX_MANIFESTS': (ManifestparserManifestList, list, - """List of manifest files defining puppeteer unit tests for Firefox. - """), - 'MARIONETTE_LAYOUT_MANIFESTS': (ManifestparserManifestList, list, """List of manifest files defining marionette-layout tests. """), diff --git a/python/mozbuild/mozbuild/testing.py b/python/mozbuild/mozbuild/testing.py index d3722fac655fb..3c95f90f8a232 100644 --- a/python/mozbuild/mozbuild/testing.py +++ b/python/mozbuild/mozbuild/testing.py @@ -277,9 +277,6 @@ def resolve_tests(self, cwd=None, **kwargs): ANDROID_INSTRUMENTATION=('instrumentation', 'instrumentation', '.', False), JETPACK_PACKAGE=('jetpack-package', 'testing/mochitest', 'jetpack-package', True), JETPACK_ADDON=('jetpack-addon', 'testing/mochitest', 'jetpack-addon', False), - FIREFOX_UI_FUNCTIONAL=('firefox-ui-functional', 'firefox-ui', '.', False), - FIREFOX_UI_UPDATE=('firefox-ui-update', 'firefox-ui', '.', False), - PUPPETEER_FIREFOX=('firefox-ui-functional', 'firefox-ui', '.', False), # marionette tests are run from the srcdir # TODO(ato): make packaging work as for other test suites diff --git a/testing/firefox-ui/mach_commands.py b/testing/firefox-ui/mach_commands.py index 368b673a22c91..9d5281d46917e 100644 --- a/testing/firefox-ui/mach_commands.py +++ b/testing/firefox-ui/mach_commands.py @@ -66,14 +66,8 @@ def run_firefox_ui_test(testtype=None, topsrcdir=None, **kwargs): if not kwargs['server_root']: kwargs['server_root'] = os.path.join(fxui_dir, 'resources') - # If called via "mach test" a dictionary of tests is passed in - if 'test_objects' in kwargs: - tests = [] - for obj in kwargs['test_objects']: - tests.append(obj['file_relpath']) - kwargs['tests'] = tests - elif not kwargs.get('tests'): - # If no tests have been selected, set default ones + # If no tests have been selected, set default ones + if not kwargs.get('tests'): kwargs['tests'] = [os.path.join(fxui_dir, 'tests', test) for test in test_types[testtype]['default_tests']] diff --git a/testing/firefox-ui/moz.build b/testing/firefox-ui/moz.build deleted file mode 100644 index 0f4a3a2e7beb0..0000000000000 --- a/testing/firefox-ui/moz.build +++ /dev/null @@ -1,11 +0,0 @@ -# This Source Code Form is subject to the terms of the Mozilla Public -# License, v. 2.0. If a copy of the MPL was not distributed with this -# file, You can obtain one at http://mozilla.org/MPL/2.0/. - -FIREFOX_UI_FUNCTIONAL_MANIFESTS += ["tests/functional/manifest.ini"] -FIREFOX_UI_UPDATE_MANIFESTS += ["tests/update/manifest.ini"] -# Bug 1272145: Move to testing/puppeteer/firefox -PUPPETEER_FIREFOX_MANIFESTS += ["tests/puppeteer/manifest.ini"] - -with Files("**"): - BUG_COMPONENT = ("Testing", "Firefox UI Tests") diff --git a/testing/mach_commands.py b/testing/mach_commands.py index 1b21300262711..4eb19a6618a45 100644 --- a/testing/mach_commands.py +++ b/testing/mach_commands.py @@ -61,16 +61,6 @@ 'mach_command': 'crashtest-ipc', 'kwargs': {'test_file': None}, }, - 'firefox-ui-functional': { - 'aliases': ('Fxfn',), - 'mach_command': 'firefox-ui-functional', - 'kwargs': {}, - }, - 'firefox-ui-update': { - 'aliases': ('Fxup',), - 'mach_command': 'firefox-ui-update', - 'kwargs': {}, - }, 'jetpack': { 'aliases': ('J',), 'mach_command': 'jetpack-test', @@ -153,14 +143,6 @@ 'mach_command': 'mochitest', 'kwargs': {'flavor': 'chrome', 'test_paths': []}, }, - 'firefox-ui-functional': { - 'mach_command': 'firefox-ui-functional', - 'kwargs': {'tests': []}, - }, - 'firefox-ui-update': { - 'mach_command': 'firefox-ui-update', - 'kwargs': {'tests': []}, - }, 'marionette': { 'mach_command': 'marionette-test', 'kwargs': {'tests': []}, diff --git a/toolkit/toolkit.mozbuild b/toolkit/toolkit.mozbuild index 05c42fa44ba31..eaae9d3d781bb 100644 --- a/toolkit/toolkit.mozbuild +++ b/toolkit/toolkit.mozbuild @@ -160,10 +160,7 @@ if 'gtk' in CONFIG['MOZ_WIDGET_TOOLKIT']: DIRS += ['/addon-sdk'] if CONFIG['ENABLE_MARIONETTE'] or CONFIG['MOZ_WIDGET_TOOLKIT'] not in ('gonk', 'android'): - DIRS += [ - '/testing/firefox-ui', - '/testing/marionette', - ] + DIRS += ['/testing/marionette'] DIRS += [ '/tools/quitter', From 6247ecc4b9595560fbd7f1fa1c1142c5753509f7 Mon Sep 17 00:00:00 2001 From: Sebastian Hengst Date: Fri, 16 Sep 2016 11:25:55 +0200 Subject: [PATCH 13/29] Backed out changeset e92048c87179 (bug 1302364) for failing test_emitter.py in e.g. spidermonkey tests. r=backout on a CLOSED TREE --- python/mozbuild/mozbuild/testing.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/python/mozbuild/mozbuild/testing.py b/python/mozbuild/mozbuild/testing.py index 3c95f90f8a232..19e3fa4fa80a2 100644 --- a/python/mozbuild/mozbuild/testing.py +++ b/python/mozbuild/mozbuild/testing.py @@ -260,12 +260,10 @@ def resolve_tests(self, cwd=None, **kwargs): # Keys are variable prefixes and values are tuples describing how these # manifests should be handled: # -# (flavor, install_root, install_subdir, package_tests) +# (flavor, install_prefix, package_tests) # # flavor identifies the flavor of this test. -# install_root is the path prefix to install the files starting from the root -# directory and not as specified by the manifest location. (bug 972168) -# install_subdir is the path of where to install the files in +# install_prefix is the path prefix of where to install the files in # the tests directory. # package_tests indicates whether to package test files into the test # package; suites that compile the test files should not install From d268a90089db7df7941b4e2902bc4b6b58fc5256 Mon Sep 17 00:00:00 2001 From: Mike de Boer Date: Fri, 16 Sep 2016 12:05:00 +0200 Subject: [PATCH 14/29] Bug 1302522 - remove CSS properties that slow down scrolling with find toolbar dimming enabled and listen to a singular scroll event. r=mstange MozReview-Commit-ID: EuFMJNTNRC0 --HG-- extra : rebase_source : 43151f8c60ec5e81d49c318b1f301d8d19a84d0d --- toolkit/modules/FinderHighlighter.jsm | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/toolkit/modules/FinderHighlighter.jsm b/toolkit/modules/FinderHighlighter.jsm index b2f39f5864af1..4ba19e4dbcc53 100644 --- a/toolkit/modules/FinderHighlighter.jsm +++ b/toolkit/modules/FinderHighlighter.jsm @@ -46,10 +46,8 @@ const kModalStyles = { ["margin", "-2px 0 0 -2px !important"], ["padding", "2px !important"], ["pointer-events", "none"], - ["transition-property", "opacity, transform, top, left"], - ["transition-duration", "50ms"], - ["transition-timing-function", "linear"], ["white-space", "nowrap"], + ["will-change", "transform"], ["z-index", 2] ], outlineNodeDebug: [ ["z-index", 2147483647] ], @@ -60,7 +58,6 @@ const kModalStyles = { ], maskNode: [ ["background", "#000"], - ["mix-blend-mode", "multiply"], ["opacity", ".35"], ["pointer-events", "none"], ["position", "absolute"], @@ -116,6 +113,9 @@ function mockAnonymousContentNode(domNode) { try { domNode.parentNode.removeChild(domNode); } catch (ex) {} + }, + setAnimationForElement(id, keyframes, duration) { + return (domNode.querySelector("#" + id) || domNode).animate(keyframes, duration); } }; } @@ -1157,8 +1157,7 @@ FinderHighlighter.prototype = { let target = this.iterator._getDocShell(window).chromeEventHandler; target.addEventListener("MozAfterPaint", dict.highlightListeners[0]); target.addEventListener("resize", dict.highlightListeners[1]); - target.addEventListener("DOMMouseScroll", dict.highlightListeners[2]); - target.addEventListener("mousewheel", dict.highlightListeners[2]); + target.addEventListener("scroll", dict.highlightListeners[2]); target.addEventListener("click", dict.highlightListeners[3]); }, @@ -1176,8 +1175,7 @@ FinderHighlighter.prototype = { let target = this.iterator._getDocShell(window).chromeEventHandler; target.removeEventListener("MozAfterPaint", dict.highlightListeners[0]); target.removeEventListener("resize", dict.highlightListeners[1]); - target.removeEventListener("DOMMouseScroll", dict.highlightListeners[2]); - target.removeEventListener("mousewheel", dict.highlightListeners[2]); + target.removeEventListener("scroll", dict.highlightListeners[2]); target.removeEventListener("click", dict.highlightListeners[3]); dict.highlightListeners = null; From bba975bca2ce9f75406aafef7bb13f2bcbbf6d05 Mon Sep 17 00:00:00 2001 From: James Graham Date: Tue, 13 Sep 2016 14:18:41 +0100 Subject: [PATCH 15/29] Bug 1302796 - Add integration between structured logging and lints, r=ahal MozReview-Commit-ID: K3tu0Zdg5go --HG-- extra : rebase_source : 2b78059908c3a297b8ce7d134af20733fc082df5 --- python/mozlint/mozlint/types.py | 33 ++++++++++++++++ python/mozlint/setup.py | 2 +- python/mozlint/test/linters/structured.lint | 28 +++++++++++++ python/mozlint/test/test_types.py | 2 +- .../mozlog/mozlog/formatters/errorsummary.py | 12 ++++++ .../mozlog/mozlog/formatters/machformatter.py | 19 +++++++++ .../mozlog/mozlog/formatters/tbplformatter.py | 6 +++ testing/mozbase/mozlog/mozlog/logtypes.py | 14 +++++++ .../mozbase/mozlog/mozlog/structuredlog.py | 39 ++++++++++++++++++- testing/mozbase/mozlog/setup.py | 2 +- tools/lint/docs/create.rst | 25 +++++++++--- 11 files changed, 172 insertions(+), 10 deletions(-) create mode 100644 python/mozlint/test/linters/structured.lint diff --git a/python/mozlint/mozlint/types.py b/python/mozlint/mozlint/types.py index e09275af47949..2f49ae2bf2e2d 100644 --- a/python/mozlint/mozlint/types.py +++ b/python/mozlint/mozlint/types.py @@ -5,8 +5,12 @@ from __future__ import unicode_literals import re +import sys from abc import ABCMeta, abstractmethod +from mozlog import get_default_logger, commandline, structuredlog +from mozlog.reader import LogHandler + from . import result from .pathutils import filterpaths @@ -101,9 +105,38 @@ def _lint(self, files, linter, **lintargs): return payload(files, **lintargs) +class LintHandler(LogHandler): + def __init__(self, linter): + self.linter = linter + self.results = [] + + def lint(self, data): + self.results.append(result.from_linter(self.linter, **data)) + + +class StructuredLogType(BaseType): + batch = True + + def _lint(self, files, linter, **lintargs): + payload = linter["payload"] + handler = LintHandler(linter) + logger = linter.get("logger") + if logger is None: + logger = get_default_logger() + if logger is None: + logger = structuredlog.StructuredLogger(linter["name"]) + commandline.setup_logging(logger, {}, {"mach": sys.stdout}) + logger.add_handler(handler) + try: + payload(files, logger, **lintargs) + except KeyboardInterrupt: + pass + return handler.results + supported_types = { 'string': StringType(), 'regex': RegexType(), 'external': ExternalType(), + 'structured_log': StructuredLogType() } """Mapping of type string to an associated instance.""" diff --git a/python/mozlint/setup.py b/python/mozlint/setup.py index c770fe83cab21..62d25c38b3be9 100644 --- a/python/mozlint/setup.py +++ b/python/mozlint/setup.py @@ -5,7 +5,7 @@ from setuptools import setup VERSION = 0.1 -DEPS = [] +DEPS = ["mozlog>=3.4"] setup( name='mozlint', diff --git a/python/mozlint/test/linters/structured.lint b/python/mozlint/test/linters/structured.lint new file mode 100644 index 0000000000000..e8be8d7b3a440 --- /dev/null +++ b/python/mozlint/test/linters/structured.lint @@ -0,0 +1,28 @@ +# -*- Mode: python; indent-tabs-mode: nil; tab-width: 40 -*- +# vim: set filetype=python: +# This Source Code Form is subject to the terms of the Mozilla Public +# License, v. 2.0. If a copy of the MPL was not distributed with this +# file, You can obtain one at http://mozilla.org/MPL/2.0/. + + +def lint(files, logger, **kwargs): + for path in files: + with open(path, 'r') as fh: + for i, line in enumerate(fh.readlines()): + if 'foobar' in line: + logger.lint_error(path=path, + lineno=i+1, + column=1, + rule="no-foobar") + + +LINTER = { + 'name': "StructuredLinter", + 'description': "It's bad to have the string foobar in js files.", + 'include': [ + 'files', + ], + 'type': 'structured_log', + 'extensions': ['.js', '.jsm'], + 'payload': lint, +} diff --git a/python/mozlint/test/test_types.py b/python/mozlint/test/test_types.py index 9df668c766e13..ee0ea9b638c74 100644 --- a/python/mozlint/test/test_types.py +++ b/python/mozlint/test/test_types.py @@ -17,7 +17,7 @@ def _path(name): return _path -@pytest.fixture(params=['string.lint', 'regex.lint', 'external.lint']) +@pytest.fixture(params=['string.lint', 'regex.lint', 'external.lint', 'structured.lint']) def linter(lintdir, request): return os.path.join(lintdir, request.param) diff --git a/testing/mozbase/mozlog/mozlog/formatters/errorsummary.py b/testing/mozbase/mozlog/mozlog/formatters/errorsummary.py index eeb50e77fa6d9..21aa684d90035 100644 --- a/testing/mozbase/mozlog/mozlog/formatters/errorsummary.py +++ b/testing/mozbase/mozlog/mozlog/formatters/errorsummary.py @@ -53,3 +53,15 @@ def crash(self, item): "stackwalk_stdout": item.get("stackwalk_stdout"), "stackwalk_stderr": item.get("stackwalk_stderr")} return self._output("crash", data) + + def lint(self, item): + data = { + "level": item["level"], + "path": item["path"], + "message": item["message"], + "lineno": item["lineno"], + "column": item.get("column"), + "rule": item.get("rule"), + "linter": item.get("linter") + } + self._output("lint", data) diff --git a/testing/mozbase/mozlog/mozlog/formatters/machformatter.py b/testing/mozbase/mozlog/mozlog/formatters/machformatter.py index e67cbb7719df9..2b9132631f2be 100644 --- a/testing/mozbase/mozlog/mozlog/formatters/machformatter.py +++ b/testing/mozbase/mozlog/mozlog/formatters/machformatter.py @@ -356,6 +356,25 @@ def log(self, data): return rv + def lint(self, data): + term = self.terminal if self.terminal is not None else NullTerminal() + fmt = "{path} {c1}{lineno}{column} {c2}{level}{normal} {message} {c1}{rule}({linter}){normal}" + message = fmt.format( + path=data["path"], + normal=term.normal, + c1=term.grey, + c2=term.red if data["level"] == 'error' else term.yellow, + lineno=str(data["lineno"]), + column=(":" + str(data["column"])) if data.get("column") else "", + level=data["level"], + message=data["message"], + rule='{} '.format(data["rule"]) if data.get("rule") else "", + linter=data["linter"].lower() if data.get("linter") else "", + ) + + return message + + def _get_subtest_data(self, data): test = self._get_test_id(data) return self.status_buffer.get(test, {"count": 0, "unexpected": [], "pass": 0}) diff --git a/testing/mozbase/mozlog/mozlog/formatters/tbplformatter.py b/testing/mozbase/mozlog/mozlog/formatters/tbplformatter.py index 76064a2ef5a38..cd8480bf6e733 100644 --- a/testing/mozbase/mozlog/mozlog/formatters/tbplformatter.py +++ b/testing/mozbase/mozlog/mozlog/formatters/tbplformatter.py @@ -233,3 +233,9 @@ def valgrind_error(self, data): rv = rv + line + "\n" return rv + + def lint(self, data): + fmt = "TEST-UNEXPECTED-{level} | {path}:{lineno}{column} | {message} ({rule})" + data["column"] = ":%s" % data["column"] if data["column"] else "" + data['rule'] = data['rule'] or data['linter'] or "" + message.append(fmt.format(**data)) diff --git a/testing/mozbase/mozlog/mozlog/logtypes.py b/testing/mozbase/mozlog/mozlog/logtypes.py index dc516c98864ec..435303b306bd7 100644 --- a/testing/mozbase/mozlog/mozlog/logtypes.py +++ b/testing/mozbase/mozlog/mozlog/logtypes.py @@ -157,6 +157,7 @@ class Dict(DataType): def convert(self, data): return dict(data) + class List(DataType): def __init__(self, name, item_type, default=no_default, optional=False): DataType.__init__(self, name, default, optional) @@ -169,6 +170,19 @@ class Int(DataType): def convert(self, data): return int(data) + class Any(DataType): def convert(self, data): return data + + +class Tuple(DataType): + def __init__(self, name, item_types, default=no_default, optional=False): + DataType.__init__(self, name, default, optional) + self.item_types = item_types + + def convert(self, data): + if len(data) != len(self.item_types): + raise ValueError("Expected %i items got %i" % (len(self.item_types), len(data))) + return tuple(item_type.convert(value) + for item_type, value in zip(self.item_types, data)) diff --git a/testing/mozbase/mozlog/mozlog/structuredlog.py b/testing/mozbase/mozlog/mozlog/structuredlog.py index fa8e3559492ce..b6be195a32929 100644 --- a/testing/mozbase/mozlog/mozlog/structuredlog.py +++ b/testing/mozbase/mozlog/mozlog/structuredlog.py @@ -11,7 +11,7 @@ import time import traceback -from logtypes import Unicode, TestId, Status, SubStatus, Dict, List, Int, Any +from logtypes import Unicode, TestId, Status, SubStatus, Dict, List, Int, Any, Tuple from logtypes import log_action, convertor_registry """Structured Logging for recording test results. @@ -93,6 +93,8 @@ def set_default_logger(default_logger): log_levels = dict((k.upper(), v) for v, k in enumerate(["critical", "error", "warning", "info", "debug"])) +lint_levels = ["ERROR", "WARNING"] + def log_actions(): """Returns the set of actions implemented by mozlog.""" return set(convertor_registry.keys()) @@ -438,11 +440,44 @@ def log(self, data): log.__name__ = str(level_name).lower() return log +def _lint_func(level_name): + @log_action(Unicode("path"), + Unicode("message", default=""), + Int("lineno", default=0), + Int("column", default=None, optional=True), + Unicode("hint", default=None, optional=True), + Unicode("source", default=None, optional=True), + Unicode("rule", default=None, optional=True), + Tuple("lineoffset", (Int, Int), default=None, optional=True), + Unicode("linter", default=None, optional=True)) + def lint(self, data): + data["level"] = level_name + self._log_data("lint", data) + lint.__doc__ = """Log an error resulting from a failed lint check + + :param linter: name of the linter that flagged this error + :param path: path to the file containing the error + :param message: text describing the error + :param lineno: line number that contains the error + :param column: column containing the error + :param hint: suggestion for fixing the error (optional) + :param source: source code context of the error (optional) + :param rule: name of the rule that was violated (optional) + :param lineoffset: denotes an error spans multiple lines, of the form + (, ) (optional) + """ + lint.__name__ = str("lint_%s" % level_name) + return lint + -# Create all the methods on StructuredLog for debug levels +# Create all the methods on StructuredLog for log/lint levels for level_name in log_levels: setattr(StructuredLogger, level_name.lower(), _log_func(level_name)) +for level_name in lint_levels: + level_name = level_name.lower() + name = "lint_%s" % level_name + setattr(StructuredLogger, name, _lint_func(level_name)) class StructuredLogFileLike(object): """Wrapper for file-like objects to redirect writes to logger diff --git a/testing/mozbase/mozlog/setup.py b/testing/mozbase/mozlog/setup.py index 2aee41e0a076d..5495af18a1c50 100644 --- a/testing/mozbase/mozlog/setup.py +++ b/testing/mozbase/mozlog/setup.py @@ -5,7 +5,7 @@ from setuptools import setup, find_packages PACKAGE_NAME = 'mozlog' -PACKAGE_VERSION = '3.3' +PACKAGE_VERSION = '3.4' setup(name=PACKAGE_NAME, version=PACKAGE_VERSION, diff --git a/tools/lint/docs/create.rst b/tools/lint/docs/create.rst index 1a4e5275940af..a132417a8ecff 100644 --- a/tools/lint/docs/create.rst +++ b/tools/lint/docs/create.rst @@ -29,17 +29,28 @@ There are three types of linters, though more may be added in the future. 1. string - fails if substring is found 2. regex - fails if regex matches 3. external - fails if a python function returns a non-empty result list +4. structured_log - fails if a mozlog logger emits any lint_error or lint_warning log messages As seen from the example above, string and regex linters are very easy to create, but they should be avoided if possible. It is much better to use a context aware linter for the language you are trying to lint. For example, use eslint to lint JavaScript files, use flake8 to lint python files, etc. -Which brings us to the third and most interesting type of linter, external. External linters call -an arbitrary python function which is responsible for not only running the linter, but ensuring the -results are structured properly. For example, an external type could shell out to a 3rd party -linter, collect the output and format it into a list of :class:`ResultContainer` objects. - +Which brings us to the third and most interesting type of linter, +external. External linters call an arbitrary python function which is +responsible for not only running the linter, but ensuring the results +are structured properly. For example, an external type could shell out +to a 3rd party linter, collect the output and format it into a list of +:class:`ResultContainer` objects. The signature for this python +function is ``lint(files, **kwargs)``, where ``files`` is a list of +files to lint. + +Structured log linters are much like external linters, but suitable +for cases where the linter code is using mozlog and emits +``lint_error`` or ``lint_warning`` logging messages when the lint +fails. This is recommended for writing novel gecko-specific lints. In +this case the signature for lint functions is ``lint(files, logger, +**kwargs)``. LINTER Definition ----------------- @@ -64,6 +75,10 @@ following additional keys may be specified: * rule - An id string for the lint rule (optional) * level - The severity of the infraction, either 'error' or 'warning' (optional) +For structured_log lints the following additional keys apply: + +* logger - A StructuredLog object to use for logging. If not supplied + one will be created (optional) Example ------- From b20e631b19254c57604a7904eebb310dff208c13 Mon Sep 17 00:00:00 2001 From: James Graham Date: Tue, 13 Sep 2016 14:20:20 +0100 Subject: [PATCH 16/29] Bug 1302796 - Add --check-clean flag to mach manifest update and mozlint integration, r=ahal,Ms2ger MozReview-Commit-ID: 8Z4ywNEbF8G --HG-- extra : rebase_source : 029995f188d14d0b70ed6e146a0feb71a046512f --- testing/web-platform/mach_commands.py | 37 +++++------ testing/web-platform/manifestupdate.py | 89 ++++++++++++++++++++++++++ tools/lint/wpt_manifest.lint | 34 ++++++++++ 3 files changed, 139 insertions(+), 21 deletions(-) create mode 100644 testing/web-platform/manifestupdate.py create mode 100644 tools/lint/wpt_manifest.lint diff --git a/testing/web-platform/mach_commands.py b/testing/web-platform/mach_commands.py index 4c41283e761c4..537bc3b90f73a 100644 --- a/testing/web-platform/mach_commands.py +++ b/testing/web-platform/mach_commands.py @@ -237,24 +237,13 @@ def run_create(self, context, **kwargs): class WPTManifestUpdater(MozbuildObject): - def run_update(self): - import imp + def run_update(self, check_clean=False, **kwargs): + import manifestupdate from wptrunner import wptlogging - from wptrunner.wptcommandline import get_test_paths, set_from_config - from wptrunner.testloader import ManifestLoader + logger = wptlogging.setup(kwargs, {"mach": sys.stdout}) wpt_dir = os.path.abspath(os.path.join(self.topsrcdir, 'testing', 'web-platform')) - - localpaths = imp.load_source("localpaths", - os.path.join(wpt_dir, "tests", "tools", "localpaths.py")) - kwargs = {"config": os.path.join(wpt_dir, "wptrunner.ini"), - "tests_root": None, - "metadata_root": None} - - wptlogging.setup({}, {"mach": sys.stdout}) - set_from_config(kwargs) - test_paths = get_test_paths(kwargs["config"]) - ManifestLoader(test_paths, force_manifest_update=True).load() + manifestupdate.update(logger, wpt_dir, check_clean) def create_parser_wpt(): @@ -292,8 +281,16 @@ def create_parser_create(): return p +def create_parser_manifest_update(): + import manifestupdate + return manifestupdate.create_parser() + + @CommandProvider class MachCommands(MachCommandBase): + def setup(self): + self._activate_virtualenv() + @Command("web-platform-tests", category="testing", conditions=[conditions.is_firefox], @@ -336,9 +333,6 @@ def update_web_platform_tests(self, **params): def update_wpt(self, **params): return self.update_web_platform_tests(**params) - def setup(self): - self._activate_virtualenv() - @Command("web-platform-tests-reduce", category="testing", conditions=[conditions.is_firefox], @@ -372,8 +366,9 @@ def create_wpt(self, **params): return self.create_web_platform_test(**params) @Command("wpt-manifest-update", - category="testing") - def wpt_manifest_update(self, **parms): + category="testing", + parser=create_parser_manifest_update) + def wpt_manifest_update(self, **params): self.setup() wpt_manifest_updater = self._spawn(WPTManifestUpdater) - wpt_manifest_updater.run_update() + return wpt_manifest_updater.run_update(**params) diff --git a/testing/web-platform/manifestupdate.py b/testing/web-platform/manifestupdate.py new file mode 100644 index 0000000000000..7b15a55eb9e43 --- /dev/null +++ b/testing/web-platform/manifestupdate.py @@ -0,0 +1,89 @@ +import argparse +import imp +import os +import sys + +from mozlog.structured import commandline +from wptrunner.wptcommandline import get_test_paths, set_from_config +from wptrunner.testloader import ManifestLoader + +def create_parser(): + p = argparse.ArgumentParser() + p.add_argument("--check-clean", action="store_true", + help="Check that updating the manifest doesn't lead to any changes") + commandline.add_logging_group(p) + + return p + + +def update(logger, wpt_dir, check_clean=True): + localpaths = imp.load_source("localpaths", + os.path.join(wpt_dir, "tests", "tools", "localpaths.py")) + kwargs = {"config": os.path.join(wpt_dir, "wptrunner.ini"), + "tests_root": None, + "metadata_root": None} + + set_from_config(kwargs) + config = kwargs["config"] + test_paths = get_test_paths(config) + + if check_clean: + old_manifests = {} + for data in test_paths.itervalues(): + path = os.path.join(data["metadata_path"], "MANIFEST.json") + with open(path) as f: + old_manifests[path] = f.readlines() + + try: + ManifestLoader(test_paths, force_manifest_update=True).load() + + rv = 0 + + if check_clean: + clean = diff_manifests(logger, old_manifests) + if not clean: + rv = 1 + finally: + if check_clean: + for path, data in old_manifests.iteritems(): + logger.info("Restoring manifest %s" % path) + with open(path, "w") as f: + f.writelines(data) + + return rv + +def diff_manifests(logger, old_manifests): + logger.info("Diffing old and new manifests") + import difflib + + clean = True + for path, old in old_manifests.iteritems(): + with open(path) as f: + new = f.readlines() + + if old != new: + clean = False + sm = difflib.SequenceMatcher(a=old, b=new) + for group in sm.get_grouped_opcodes(): + logged = False + message = [] + for op, old_0, old_1, new_0, new_1 in group: + if op != "equal" and not logged: + logged = True + logger.lint_error(path=path, + message="Manifest changed", + lineno=(old_0 + 1), + source="\n".join(old[old_0:old_1]), + linter="wpt-manifest") + if op == "equal": + message.extend(' ' + line for line in old[old_0:old_1]) + if op in ('replace', 'delete'): + message.extend('-' + line for line in old[old_0:old_1]) + if op in ('replace', 'insert'): + message.extend('+' + line for line in new[new_0:new_1]) + logger.info("".join(message)) + if clean: + logger.info("No differences found") + + return clean + diff --git a/tools/lint/wpt_manifest.lint b/tools/lint/wpt_manifest.lint new file mode 100644 index 0000000000000..584b33b4e4d81 --- /dev/null +++ b/tools/lint/wpt_manifest.lint @@ -0,0 +1,34 @@ +# -*- Mode: python; indent-tabs-mode: nil; tab-width: 40 -*- +# vim: set filetype=python: +# This Source Code Form is subject to the terms of the Mozilla Public +# License, v. 2.0. If a copy of the MPL was not distributed with this +# file, You can obtain one at http://mozilla.org/MPL/2.0/. + +import imp +import json +import os +import sys + +from mozprocess import ProcessHandler + +from mozlint import result + + +def lint(files, logger, **kwargs): + wpt_dir = os.path.join(kwargs["root"], "testing", "web-platform") + manifestupdate = imp.load_source("manifestupdate", + os.path.join(wpt_dir, "manifestupdate.py")) + manifestupdate.update(logger, wpt_dir, True) + + +LINTER = { + 'name': "wpt_manifest", + 'description': "web-platform-tests manifest lint", + 'include': [ + 'testing/web-platform/tests', + 'testing/web-platform/mozilla/tests', + ], + 'exclude': [], + 'type': 'structured_log', + 'payload': lint, +} From bdf8391e8f9b1282da4e88e5876e10023212df94 Mon Sep 17 00:00:00 2001 From: James Graham Date: Fri, 16 Sep 2016 10:24:48 +0100 Subject: [PATCH 17/29] Bug 1302796 - Add wpt manifest lint to taskcluster, r=ahal MozReview-Commit-ID: IpNUVTC35U1 --HG-- extra : rebase_source : a87fbb242fd62b6a4b5c9e9540e56b889403d39d --- taskcluster/ci/source-check/mozlint.yml | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/taskcluster/ci/source-check/mozlint.yml b/taskcluster/ci/source-check/mozlint.yml index 55216d09ba3e4..174bfefa5320c 100644 --- a/taskcluster/ci/source-check/mozlint.yml +++ b/taskcluster/ci/source-check/mozlint.yml @@ -82,14 +82,16 @@ wptlint-gecko/opt: max-run-time: 1800 run: using: mach - mach: lint -l wpt -f treeherder + mach: lint -l wpt -l wpt_manifest -f treeherder run-on-projects: - integration - release when: files-changed: - 'testing/web-platform/tests/**' + - 'testing/web-platform/mozilla/tests/**' + - 'testing/web-platform/meta/MANIFEST.json' + - 'testing/web-platform/mozilla/meta/MANIFEST.json' - 'python/mozlint/**' - 'tools/lint/**' - 'testing/docker/lint/**' - From 12c974d5c68c8c1e2287f1d5a51429e2a5f57f23 Mon Sep 17 00:00:00 2001 From: Julian Descottes Date: Thu, 15 Sep 2016 17:07:38 +0200 Subject: [PATCH 18/29] Bug 1303043 - fix toolbox HTML tooltips in devtools window host;r=Honza MozReview-Commit-ID: 96LLMJngQOX --HG-- extra : rebase_source : 23a7c0371b2fc4394f36d8d58fa81eea5f21938f --- devtools/client/framework/toolbox-window.xul | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/devtools/client/framework/toolbox-window.xul b/devtools/client/framework/toolbox-window.xul index 7256a57f7929f..cd14a3597c486 100644 --- a/devtools/client/framework/toolbox-window.xul +++ b/devtools/client/framework/toolbox-window.xul @@ -42,5 +42,6 @@ command="toolbox-cmd-close"/> - + + From 95dbb8627b6489855abe9c806718254cf87a654e Mon Sep 17 00:00:00 2001 From: Marco Bonardo Date: Wed, 7 Sep 2016 17:44:46 +0200 Subject: [PATCH 19/29] Bug 1301093 - Part 1: don't wait for a result when we won't get one. r=adw MozReview-Commit-ID: E6l9mW3ZoKp --HG-- extra : rebase_source : 39144a190f27257b190178308b26fa3f7bea15a2 --- .../urlbar/browser_autocomplete_enter_race.js | 40 ++++++++++++++----- browser/base/content/urlbarBindings.xml | 11 +++-- .../autocomplete/nsAutoCompleteController.cpp | 12 ++++-- .../nsIAutoCompleteController.idl | 21 ++++++---- .../satchel/nsFormFillController.cpp | 16 +++++--- 5 files changed, 70 insertions(+), 30 deletions(-) diff --git a/browser/base/content/test/urlbar/browser_autocomplete_enter_race.js b/browser/base/content/test/urlbar/browser_autocomplete_enter_race.js index 98811165d5ab1..19fd2f5b6d23c 100644 --- a/browser/base/content/test/urlbar/browser_autocomplete_enter_race.js +++ b/browser/base/content/test/urlbar/browser_autocomplete_enter_race.js @@ -1,23 +1,43 @@ -add_task(function*() { - registerCleanupFunction(function* () { - yield PlacesUtils.bookmarks.remove(bm); - }); +// The order of these tests matters! +add_task(function* setup () { + let tab = yield BrowserTestUtils.openNewForegroundTab(gBrowser); let bm = yield PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.unfiledGuid, url: "http://example.com/?q=%s", title: "test" }); + registerCleanupFunction(function* () { + yield PlacesUtils.bookmarks.remove(bm); + yield BrowserTestUtils.removeTab(tab); + }); yield PlacesUtils.keywords.insert({ keyword: "keyword", url: "http://example.com/?q=%s" }); + // Needs at least one success. + ok(true, "Setup complete"); +}); - yield new Promise(resolve => waitForFocus(resolve, window)); - +add_task(function* test_keyword() { yield promiseAutocompleteResultPopup("keyword bear"); gURLBar.focus(); EventUtils.synthesizeKey("d", {}); EventUtils.synthesizeKey("VK_RETURN", {}); + info("wait for the page to load"); + yield BrowserTestUtils.browserLoaded(gBrowser.selectedTab.linkedBrowser, + false, "http://example.com/?q=beard"); +}); + +add_task(function* test_sametext() { + yield promiseAutocompleteResultPopup("example.com", window, true); + + // Simulate re-entering the same text searched the last time. This may happen + // through a copy paste, but clipboard handling is not much reliable, so just + // fire an input event. + info("synthesize input event"); + let event = document.createEvent("Events"); + event.initEvent("input", true, true); + gURLBar.dispatchEvent(event); + EventUtils.synthesizeKey("VK_RETURN", {}); - yield promiseTabLoadEvent(gBrowser.selectedTab); - is(gBrowser.selectedBrowser.currentURI.spec, - "http://example.com/?q=beard", - "Latest typed characters should have been used"); + info("wait for the page to load"); + yield BrowserTestUtils.browserLoaded(gBrowser.selectedTab.linkedBrowser, + false, "http://example.com/"); }); diff --git a/browser/base/content/urlbarBindings.xml b/browser/base/content/urlbarBindings.xml index b9b300774081e..e649e1b3659c1 100644 --- a/browser/base/content/urlbarBindings.xml +++ b/browser/base/content/urlbarBindings.xml @@ -1005,8 +1005,12 @@ file, You can obtain one at http://mozilla.org/MPL/2.0/. this._value = this.inputField.value; gBrowser.userTypedValue = this.value; this.valueIsTyped = true; - this.gotResultForCurrentQuery = false; - this.mController.handleText(); + // Only wait for a result when we are sure to get one. In some + // cases, like when pasting the same exact text, we may not fire + // a new search and we won't get a result. + if (this.mController.handleText()) { + this.gotResultForCurrentQuery = false; + } } this.resetActionType(); ]]> @@ -1053,7 +1057,8 @@ file, You can obtain one at http://mozilla.org/MPL/2.0/. // a backspace on the text value instead of removing the result. if (this.popup.selectedIndex == 0 && this.popup._isFirstResultHeuristic) { - return this.mController.handleText(); + this.mController.handleText(); + return false; } return this.mController.handleDelete(); ]]> diff --git a/toolkit/components/autocomplete/nsAutoCompleteController.cpp b/toolkit/components/autocomplete/nsAutoCompleteController.cpp index 2f521bc394d10..29bc06e391da3 100644 --- a/toolkit/components/autocomplete/nsAutoCompleteController.cpp +++ b/toolkit/components/autocomplete/nsAutoCompleteController.cpp @@ -180,8 +180,9 @@ nsAutoCompleteController::StartSearch(const nsAString &aSearchString) } NS_IMETHODIMP -nsAutoCompleteController::HandleText() +nsAutoCompleteController::HandleText(bool *_retval) { + *_retval = false; // Note: the events occur in the following order when IME is used. // 1. a compositionstart event(HandleStartComposition) // 2. some input events (HandleText), eCompositionState_Composing @@ -284,6 +285,7 @@ nsAutoCompleteController::HandleText() return NS_OK; } + *_retval = true; StartSearches(); return NS_OK; @@ -623,7 +625,8 @@ nsAutoCompleteController::HandleDelete(bool *_retval) input->GetPopupOpen(&isOpen); if (!isOpen || mRowCount <= 0) { // Nothing left to delete, proceed as normal - HandleText(); + bool unused = false; + HandleText(&unused); return NS_OK; } @@ -634,7 +637,8 @@ nsAutoCompleteController::HandleDelete(bool *_retval) popup->GetSelectedIndex(&index); if (index == -1) { // No row is selected in the list - HandleText(); + bool unused = false; + HandleText(&unused); return NS_OK; } @@ -1193,7 +1197,7 @@ nsAutoCompleteController::StartSearch(uint16_t aSearchType) nsAutoString searchParam; nsresult rv = input->GetSearchParam(searchParam); if (NS_FAILED(rv)) - return rv; + return rv; // FormFill expects the searchParam to only contain the input element id, // other consumers may have other expectations, so this modifies it only diff --git a/toolkit/components/autocomplete/nsIAutoCompleteController.idl b/toolkit/components/autocomplete/nsIAutoCompleteController.idl index f4726b42650f4..ada87f2b10d84 100644 --- a/toolkit/components/autocomplete/nsIAutoCompleteController.idl +++ b/toolkit/components/autocomplete/nsIAutoCompleteController.idl @@ -38,7 +38,7 @@ interface nsIAutoCompleteController : nsISupports */ void startSearch(in AString searchString); - /* + /* * Stop all asynchronous searches */ void stopSearch(); @@ -55,8 +55,10 @@ interface nsIAutoCompleteController : nsISupports * it's not in composing mode. DOM compositionend event is not good * timing for calling handleText(). DOM input event immediately after * DOM compositionend event is the best timing to call this. + * + * @return whether this handler started a new search. */ - void handleText(); + boolean handleText(); /* * Notify the controller that the user wishes to enter the current text. If @@ -70,7 +72,8 @@ interface nsIAutoCompleteController : nsISupports * The event that triggered the enter, like a key event if the user * pressed the Return key or a click event if the user clicked a popup * item. - * @return True if the controller wishes to prevent event propagation and default event + * @return Whether the controller wishes to prevent event propagation and + * default event. */ boolean handleEnter(in boolean aIsPopupSelection, [optional] in nsIDOMEvent aEvent); @@ -78,7 +81,8 @@ interface nsIAutoCompleteController : nsISupports /* * Notify the controller that the user wishes to revert autocomplete * - * @return True if the controller wishes to prevent event propagation and default event + * @return Whether the controller wishes to prevent event propagation and + * default event. */ boolean handleEscape(); @@ -98,7 +102,7 @@ interface nsIAutoCompleteController : nsISupports */ void handleEndComposition(); - /* + /* * Handle tab. Just closes up. */ void handleTab(); @@ -107,16 +111,19 @@ interface nsIAutoCompleteController : nsISupports * Notify the controller of the following key navigation events: * up, down, left, right, page up, page down * - * @return True if the controller wishes to prevent event propagation and default event + * @return Whether the controller wishes to prevent event propagation and + * default event */ boolean handleKeyNavigation(in unsigned long key); /* * Notify the controller that the user chose to delete the current * auto-complete result. + * + * @return Whether the controller removed a result item. */ boolean handleDelete(); - + /* * Get the value of the result at a given index in the last completed search */ diff --git a/toolkit/components/satchel/nsFormFillController.cpp b/toolkit/components/satchel/nsFormFillController.cpp index 40208f68e9d08..e108e8c2eea25 100644 --- a/toolkit/components/satchel/nsFormFillController.cpp +++ b/toolkit/components/satchel/nsFormFillController.cpp @@ -825,8 +825,9 @@ nsFormFillController::HandleEvent(nsIDOMEvent* aEvent) return KeyPress(aEvent); } if (type.EqualsLiteral("input")) { + bool unused = false; return (!mSuppressOnInput && mController && mFocusedInput) ? - mController->HandleText() : NS_OK; + mController->HandleText(&unused) : NS_OK; } if (type.EqualsLiteral("blur")) { if (mFocusedInput) @@ -936,6 +937,7 @@ nsFormFillController::KeyPress(nsIDOMEvent* aEvent) return NS_ERROR_FAILURE; bool cancel = false; + bool unused = false; uint32_t k; keyEvent->GetKeyCode(&k); @@ -945,7 +947,7 @@ nsFormFillController::KeyPress(nsIDOMEvent* aEvent) mController->HandleDelete(&cancel); break; case nsIDOMKeyEvent::DOM_VK_BACK_SPACE: - mController->HandleText(); + mController->HandleText(&unused); break; #else case nsIDOMKeyEvent::DOM_VK_BACK_SPACE: @@ -953,10 +955,11 @@ nsFormFillController::KeyPress(nsIDOMEvent* aEvent) bool isShift = false; keyEvent->GetShiftKey(&isShift); - if (isShift) + if (isShift) { mController->HandleDelete(&cancel); - else - mController->HandleText(); + } else { + mController->HandleText(&unused); + } break; } @@ -1066,7 +1069,8 @@ nsFormFillController::MouseDown(nsIDOMEvent* aEvent) if (value.Length() > 0) { // Show the popup with a filtered result set mController->SetSearchString(EmptyString()); - mController->HandleText(); + bool unused = false; + mController->HandleText(&unused); } else { // Show the popup with the complete result set. Can't use HandleText() // because it doesn't display the popup if the input is blank. From 127b13e0692f7e7448d6daa259c031ad3b8b5479 Mon Sep 17 00:00:00 2001 From: Marco Bonardo Date: Wed, 7 Sep 2016 18:22:23 +0200 Subject: [PATCH 20/29] Bug 1301093 - Part 2: sanity check that delayed Enter applies to the expected search string. r=adw MozReview-Commit-ID: KiZJeztffR8 --HG-- extra : rebase_source : b1175292d466c9d00f04ff57bfbe0522c2f7e062 --- browser/base/content/urlbarBindings.xml | 41 +++++++++++++++++-------- 1 file changed, 28 insertions(+), 13 deletions(-) diff --git a/browser/base/content/urlbarBindings.xml b/browser/base/content/urlbarBindings.xml index e649e1b3659c1..241abdcfa5aac 100644 --- a/browser/base/content/urlbarBindings.xml +++ b/browser/base/content/urlbarBindings.xml @@ -126,7 +126,13 @@ file, You can obtain one at http://mozilla.org/MPL/2.0/. "" false - false + + + "" - "" + null