Closed
Bug 1083167
Opened 10 years ago
Closed 10 years ago
FormHistory.jsm: "update op='bump' does not correctly reference a entry" for empty search terms
Categories
(Firefox :: Search, defect)
Tracking
()
People
(Reporter: toadyshadow101, Assigned: adw)
References
Details
(Keywords: regression)
Attachments
(3 files, 2 obsolete files)
(deleted),
patch
|
MattN
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
adw
:
review+
lmandel
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
adw
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:33.0) Gecko/20100101 Firefox/33.0 Build ID: 20141011015303 Steps to reproduce: Open firefox 33 On about:home page (Firefox start page) press the search button with no text in the search field. Actual results: [Exception... "update op='bump' does not correctly reference a entry." nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)" location: "JS frame :: resource://gre/modules/FormHistory.jsm :: formHistoryUpdate :: line 885" data: no] ContentSearch.jsm line:142 Expected results: not thrown an error
I don't see that error. Could test: 1) in safe mode: https://support.mozilla.org/en-US/kb/troubleshoot-firefox-issues-using-safe-mode 2) with a clean profile: https://support.mozilla.org/en-US/kb/profile-manager-create-and-remove-firefox-profiles Does it fix it?
Flags: needinfo?(toadyshadow101)
Hi loic Please look in the browser console, This was tested on a clean profile on a fresh installation of windows 7 x64 os. When restarting in safe mode to test the browser became unresponsive and started lagging by about 6 seconds when dragging the window and activating Firefox menus. This error in safe mode appeared in the browser console: TypeError: asm.js type error: Disabled by javascript.options.asmjs in about:config about:home The above reported error also appeared when pressing the search button with no text in the search field.
Flags: needinfo?(toadyshadow101)
I reproduced it. Regression range: good=2014-08-02 bad=2014-08-03 http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a5a720259d79&tochange=e6614d8d85f9 Suspected bug: Bug 612453
Blocks: 612453
status-firefox33:
--- → affected
tracking-firefox34:
--- → ?
tracking-firefox35:
--- → ?
tracking-firefox36:
--- → ?
Component: Untriaged → Search
Flags: needinfo?(adw)
Keywords: regression
Comment 4•10 years ago
|
||
cc Bryan for awareness Does the page still submit the search? i.e. Is the issue only that an error is thrown or is this visible by broken functionality when searching from about:home?
status-firefox34:
--- → affected
status-firefox35:
--- → affected
status-firefox36:
--- → affected
Flags: needinfo?(toadyshadow101)
Comment 5•10 years ago
|
||
If a "bump" doesn't work then it means we're aren't recording the search term in form history (either adding an entry if it's new or updating the last use timestamp + use count if it's an existing form history entry). The code causing this is: https://mxr.mozilla.org/mozilla-central/source/toolkit/components/satchel/FormHistory.jsm?rev=0853b374d32a&mark=855-858,880-881,886-890#844 change.value is falsy on line 857 so we consider this an invalid change that we don't care about. I don't think it's useful/wanted to store an empty string in form history. We didn't allow empty string searches in 31 either so I don't think this is a regression. Lawrence, since this is only for empty searches and it's just a console warning and not a regression, I don't think this needs tracking for any version.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(adw)
OS: Windows 7 → All
Hardware: x86_64 → All
Summary: FormHistory.jsm Exception about:home search → FormHistory.jsm: "update op='bump' does not correctly reference a entry" for empty search terms
Hi Lawrence Yes, Searching and the keyword drop down function correctly, The error only logs to console if you press the search button when the text input is empty, Same on the about:newtab search.
Flags: needinfo?(toadyshadow101)
Comment 7•10 years ago
|
||
In that case, this issue is not significant enough to track but is something that we would certainly take a fix for.
Comment 9•10 years ago
|
||
This seems consistent with search.xml's behavior: http://hg.mozilla.org/mozilla-central/annotate/c0ddb1b098ec/browser/components/search/content/search.xml#l483
Assignee | ||
Comment 10•10 years ago
|
||
It's a simple fix. This also replaces the msg.target.contentWindow check with a currentURI check. We used to check contentWindow here before calling PrivateBrowsingUtils.isWindowPrivate(msg.target.contentWindow), but that was changed to PrivateBrowsingUtils.isBrowserPrivate(msg.target) by http://hg.mozilla.org/mozilla-central/diff/324798b60ba3/browser/modules/ContentSearch.jsm.
Assignee: gavin.sharp → adw
Attachment #8512825 -
Flags: review?(MattN+bmo)
Assignee | ||
Comment 11•10 years ago
|
||
Whoops, sorry Gavin. That's what I get for having the attach-new-patch page open since yesterday and not refreshing the main bug page. :-/
Assignee: adw → gavin.sharp
Assignee | ||
Comment 12•10 years ago
|
||
Comment on attachment 8512824 [details] [diff] [review] patch Review of attachment 8512824 [details] [diff] [review]: ----------------------------------------------------------------- Oh, you posted your patch only 30 seconds before I posted mine. I don't feel so bad now. :-P :-) I like my patch better... it patches code closer to the problem, so any ContentSearch client sending the AddFormHistoryEntry message with an empty string will work correctly. Plus it fixes the now unnecessary contentWindow CPOW getter. So is it OK if I leave my r? for Matt? Or do you have an opinion on my patch?
Attachment #8512824 -
Flags: review?(adw)
Comment 13•10 years ago
|
||
Comment on attachment 8512825 [details] [diff] [review] Don't pass an empty string to FormHistory.update Review of attachment 8512825 [details] [diff] [review]: ----------------------------------------------------------------- Could you add more context to your patch in the future? 3 lines isn't enough. ::: browser/modules/ContentSearch.jsm @@ +276,5 @@ > + // isBrowserPrivate() assumes that the passed-in browser is alive, but due > + // to the asynchronous nature of messaging, that may not be the case here. > + // If the browser doesn't have a currentURI, skip the call. > + if (!entry || > + !msg.target.currentURI || So the currentURI check is now to make sure the page is still alive? I don't see why we need that since I don't see why it should prevent use from adding to form history.
Comment 14•10 years ago
|
||
FWIW, I also like adw's patch better because of where it's fixing the problem.
Assignee | ||
Comment 15•10 years ago
|
||
(In reply to Matthew N. [:MattN] (behind on reviews - focused on Loop) from comment #13) > Could you add more context to your patch in the future? 3 lines isn't enough. Sorry, instead of generating a diff -U8 like usual, I attached the patch right from .hg/patches, which doesn't have a lot of context apparently. > So the currentURI check is now to make sure the page is still alive? I don't > see why we need that since I don't see why it should prevent use from adding > to form history. It's needed to short-circuit the PrivateBrowsingUtils.isBrowserPrivate(msg.target) call. If the page is in a private browsing window, we skip adding to form history. If the browser was destroyed before the message reached ContentSearch, then PrivateBrowsingUtils won't be able to tell whether the browser is private -- actually, PrivateBrowsingUtils.isBrowserPrivate will end up throwing an exception at least in the non-e10s case because browser.contentWindow will be undefined. If we can't tell whether the browser is private, then assume it is and skip adding to form history.
Assignee | ||
Comment 16•10 years ago
|
||
(In reply to Drew Willcoxon :adw from comment #15) > actually, PrivateBrowsingUtils.isBrowserPrivate will end up throwing an > exception at least in the non-e10s case because browser.contentWindow will > be undefined. Maybe I should just try-catch the call then?
Assignee | ||
Comment 17•10 years ago
|
||
Stealing bug from Gavin as we discussed on IRC. This also uses entry === "" instead of !entry so that we still log errors in cases where entry is falsey but not a string, as you requested on IRC. (In reply to Drew Willcoxon :adw from comment #15) > Sorry, instead of generating a diff -U8 like usual, I attached the patch > right from .hg/patches, which doesn't have a lot of context apparently. Ah, you can set unified=8 in the [diff] section of your .hgrc to fix that.
Assignee: gavin.sharp → adw
Attachment #8512824 -
Attachment is obsolete: true
Attachment #8512825 -
Attachment is obsolete: true
Attachment #8512825 -
Flags: review?(MattN+bmo)
Attachment #8512858 -
Flags: review?(MattN+bmo)
Updated•10 years ago
|
Iteration: --- → 36.2
Points: --- → 2
Flags: firefox-backlog+
Comment 18•10 years ago
|
||
Comment on attachment 8512858 [details] [diff] [review] Don't pass an empty string to FormHistory.update + try-catch isBrowserPrivate() Review of attachment 8512858 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/modules/ContentSearch.jsm @@ +279,5 @@ > + // properties, which won't be true if the browser has been destroyed. > + // That may be the case here due to the asynchronous nature of messaging. > + isPrivate = PrivateBrowsingUtils.isBrowserPrivate(msg.target); > + } > + catch (err) {} Nit: cuddle the catch: } catch (err) {}
Attachment #8512858 -
Flags: review?(MattN+bmo) → review+
Updated•10 years ago
|
Flags: qe-verify?
Updated•10 years ago
|
Flags: qe-verify? → qe-verify+
https://hg.mozilla.org/mozilla-central/rev/8546940128b3
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Updated•10 years ago
|
QA Contact: petruta.rasa
Assignee | ||
Comment 21•10 years ago
|
||
(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #7) > In that case, this issue is not significant enough to track but is something > that we would certainly take a fix for. The m-c patch didn't apply cleanly because the PrivateBrowsingUtils.isWindowPrivate -> isBrowserPrivate caller change landed on 35 (Aurora), in bug 1069059. This patch still calls the newer isBrowserPrivate, not isWindowPrivate, because that is available on 34 (Beta) due to https://hg.mozilla.org/releases/mozilla-beta/rev/34c81d98d96a. Approval Request Comment [Feature/regressing bug #]: Search suggestions on about:home, bug 612453 [User impact if declined]: A harmless error reported in the browser console when you click the Search button on about:home/newtab with an empty search box [Describe test coverage new/current, TBPL]: We have automated tests for this feature, which I ran on beta locally before requesting approval. I also tested manually locally. [Risks and why]: If we didn't have test coverage, moderate risk, since this touches the code that handles searching on about:home/newtab. But given that I tested this as I just described, I'd say low risk. The patch is also pretty small. [String/UUID change made/needed]: None
Attachment #8513676 -
Flags: review+
Attachment #8513676 -
Flags: approval-mozilla-beta?
Assignee | ||
Comment 22•10 years ago
|
||
This is the patch that landed on m-c. It differs from the previously posted patch in that it includes Matt's catch-cuddling nit from comment 18. Approval Request Comment: Please see comment 21. However, unlike in comment 21, I didn't bother running tests or manually testing locally because this patch applies cleanly to m-a and I wouldn't expect any behavior differences from m-c.
Attachment #8513678 -
Flags: review+
Attachment #8513678 -
Flags: approval-mozilla-aurora?
Updated•10 years ago
|
Attachment #8513678 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•10 years ago
|
Comment 24•10 years ago
|
||
Comment on attachment 8513676 [details] [diff] [review] Beta/34 patch Beta+
Attachment #8513676 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 26•10 years ago
|
||
Verified as fixed using Firefox 34 beta 7, latest Aurora 35.0a2 and latest Nightly 36.0a1 2014-11-06 under Win 7 64-bit, Ubuntu 14.04 64-bit and Mac OSX 10.9.5.
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•